Refactor pipelinerun resolution to clarify status

pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"strconv"
 
-	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"knative.dev/pkg/apis"
 
@@ -70,15 +69,9 @@ type ResolvedPipelineRunTask struct {
 	ResolvedConditionChecks TaskConditionCheckState // Could also be a TaskRun or maybe just a Pod?
 }
 
-func (t ResolvedPipelineRunTask) IsDone() bool {
-	if t.TaskRun == nil || t.PipelineTask == nil {
-		return false
-	}
-
-	status := t.TaskRun.Status.GetCondition(apis.ConditionSucceeded)
-	retriesDone := len(t.TaskRun.Status.RetriesStatus)
-	retries := t.PipelineTask.Retries
-	return status.IsTrue() || status.IsFalse() && retriesDone >= retries
+// IsDone returns true only if the task is skipped, succeeded or failed
+func (t ResolvedPipelineRunTask) IsDone(facts *PipelineRunFacts) bool {
+	return t.Skip(facts) || t.IsSuccessful() || t.IsFailure()
 }
 
 // IsSuccessful returns true only if the taskrun itself has completed successfully
@@ -87,11 +80,7 @@ func (t ResolvedPipelineRunTask) IsSuccessful() bool {
 		return false
 	}
 	c := t.TaskRun.Status.GetCondition(apis.ConditionSucceeded)
-	if c == nil {
-		return false
-	}
-
-	return c.Status == corev1.ConditionTrue
+	return c.IsTrue()
 }
 
 // IsFailure returns true only if the taskrun itself has failed
@@ -135,12 +124,9 @@ func (t ResolvedPipelineRunTask) IsStarted() bool {
 
 func (t *ResolvedPipelineRunTask) checkParentsDone(facts *PipelineRunFacts) bool {
 	stateMap := facts.State.ToMap()
-	// check if parent tasks are done executing,
-	// if any of the parents is not yet scheduled or still running,
-	// wait for it to complete before evaluating when expressions
 	node := facts.TasksGraph.Nodes[t.PipelineTask.Name]
 	for _, p := range node.Prev {
-		if !stateMap[p.Task.HashKey()].IsDone() {
+		if !stateMap[p.Task.HashKey()].IsDone(facts) {
 			return false
 		}
 	}

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

@@ -306,16 +306,7 @@ func (facts *PipelineRunFacts) successfulOrSkippedDAGTasks() []string {
 func (facts *PipelineRunFacts) checkTasksDone(d *dag.Graph) bool {
 	for _, t := range facts.State {
 		if isTaskInGraph(t.PipelineTask.Name, d) {
-			if t.TaskRun == nil {
-				// this task might have skipped if taskRun is nil
-				// continue and ignore if this task was skipped
-				// skipped task is considered part of done
-				if t.Skip(facts) {
-					continue
-				}
-				return false
-			}
-			if !t.IsDone() {
+			if !t.IsDone(facts) {
 				return false
 			}
 		}

pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go

@@ -154,12 +154,12 @@ func TestPipelineRunFacts_CheckDAGTasksDoneDone(t *testing.T) {
 
 			isDone := facts.checkTasksDone(d)
 			if d := cmp.Diff(isDone, tc.expected); d != "" {
-				t.Errorf("Didn't get expected IsDone %s", diff.PrintWantGot(d))
+				t.Errorf("Didn't get expected checkTasksDone %s", diff.PrintWantGot(d))
 			}
 			for i, pt := range tc.state {
-				isDone = pt.IsDone()
+				isDone = pt.IsDone(&facts)
 				if d := cmp.Diff(isDone, tc.ptExpected[i]); d != "" {
-					t.Errorf("Didn't get expected (ResolvedPipelineRunTask) checkTasksDone %s", diff.PrintWantGot(d))
+					t.Errorf("Didn't get expected (ResolvedPipelineRunTask) IsDone %s", diff.PrintWantGot(d))
 				}
 
 			}