Refactor pipelinerun resolution to clarify status

Refactoring the resolution, which includes:
- removing ambiguity on what `Done` means; it had different meanings in
different functions i.e. sometimes it meant that the status is not unknown,
sometimes it meant that the status is successful or failure, and sometimes
it meant that the status is successful or failure or skipped
- simplifying the function that checks if a pipelinetask is successful

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
 		}
 	}
@@ -171,7 +157,8 @@ func (t *ResolvedPipelineRunTask) Skip(facts *PipelineRunFacts) bool {
 		}
 	}
 
-	// Check if the when expressions are false, based on the input's relationship to the values
+	// Check if parent tasks are done executing and when expressions are false
+	// if so, the task is skipped
 	if t.checkParentsDone(facts) {
 		if len(t.PipelineTask.WhenExpressions) > 0 {
 			if !t.PipelineTask.WhenExpressions.HaveVariables() {

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()
-				if d := cmp.Diff(isDone, tc.ptExpected[i]); d != "" {
-					t.Errorf("Didn't get expected (ResolvedPipelineRunTask) checkTasksDone %s", diff.PrintWantGot(d))
+				isSuccessfulOrFailure := pt.IsSuccessful() || pt.IsFailure()
+				if d := cmp.Diff(isSuccessfulOrFailure, tc.ptExpected[i]); d != "" {
+					t.Errorf("Didn't get expected (ResolvedPipelineRunTask) IsSuccessful || IsFailure %s", diff.PrintWantGot(d))
 				}
 
 			}