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)) } }