From 3185d055fa0959830d3c986407a1e6e58f074003 Mon Sep 17 00:00:00 2001
From: Jerop <jerop@google.com>
Date: Wed, 7 Oct 2020 20:03:53 -0400
Subject: [PATCH] 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 (1) the status is not
unknown, (2) the status is successful or failure, and (3) the status is
successful or failure or skipped -- updated so that it's consistently that
the status is successful or failure or skipped
- simplifying the function that checks if a pipelinetask is successful
---
 .../resources/pipelinerunresolution.go        | 24 ++++---------------
 .../pipelinerun/resources/pipelinerunstate.go | 11 +--------
 .../resources/pipelinerunstate_test.go        |  6 ++---
 3 files changed, 9 insertions(+), 32 deletions(-)

diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
index 4daf112f9c3..45a11dba22b 100644
--- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
+++ b/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
 		}
 	}
diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go
index 87e70726183..21913b5a1f1 100644
--- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go
+++ b/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
 			}
 		}
diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
index 3d708da002b..878c2b7b0d7 100644
--- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
+++ b/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))
 				}
 
 			}