don't return validation error when final tasks failed/skipped

This commit closes #6139, in previous fix PR:
#6395, only dagTasks statuses
are considered and final tasks are missing. This PR fixes this.

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

examples/v1/pipelineruns/6139-regression.yaml

@@ -7,6 +7,8 @@ spec:
     params:
       - name: say-hello
         default: 'false'
+      - name: do-shutdown
+        default: 'false'
     tasks:
       - name: hello
         taskSpec:
@@ -35,11 +37,28 @@ spec:
           - input: $(params.say-hello)
             operator: in
             values: ["true"]
-
+    finally:
+      - name: shutdown
+        taskSpec:
+          results:
+            - name: result-three
+          steps:
+          - image: alpine
+            script: |
+              #!/bin/sh
+              echo "Shutdown world!"
+              echo -n "RES3" > $(results.result-three.path)
+        when:
+          - input: $(params.do-shutdown)
+            operator: in
+            values: ["true"]
     results:
       - name: result-hello
         description: Result one
         value: '$(tasks.hello.results.result-one)'
       - name: result-goodbye
         description: Result two
         value: '$(tasks.goodbye.results.result-two)'
+      - name: result-shutdown
+        description: Result shutdown
+        value: '$(finally.shutdown.results.result-three)'

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -776,9 +776,14 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1.PipelineRun, getPipel
 	pr.Status.ChildReferences = pipelineRunFacts.GetChildReferences()
 
 	pr.Status.SkippedTasks = pipelineRunFacts.GetSkippedTasks()
+
+	taskStatus := pipelineRunFacts.GetPipelineTaskStatus()
+	finalTaskStatus := pipelineRunFacts.GetPipelineFinalTaskStatus()
+	taskStatus = kmap.Union(taskStatus, finalTaskStatus)
+
 	if after.Status == corev1.ConditionTrue || after.Status == corev1.ConditionFalse {
 		pr.Status.Results, err = resources.ApplyTaskResultsToPipelineResults(ctx, pipelineSpec.Results,
-			pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pipelineRunFacts.GetPipelineTaskStatus())
+			pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), taskStatus)
 		if err != nil {
 			pr.Status.MarkFailed(v1.PipelineRunReasonFailedValidation.String(), err.Error())
 			return err

pkg/reconciler/pipelinerun/resources/pipelinerunstate.go

@@ -558,6 +558,30 @@ func (facts *PipelineRunFacts) GetPipelineTaskStatus() map[string]string {
 	return tStatus
 }
 
+// GetPipelineTaskStatus returns the status of a PipelineFinalTask depending on its taskRun
+func (facts *PipelineRunFacts) GetPipelineFinalTaskStatus() map[string]string {
+	// construct a map of tasks.<pipelineTask>.status and its state
+	tStatus := make(map[string]string)
+	for _, t := range facts.State {
+		if facts.isFinalTask(t.PipelineTask.Name) {
+			var s string
+			switch {
+			// execution status is Succeeded when a task has succeeded condition with status set to true
+			case t.isSuccessful():
+				s = v1.TaskRunReasonSuccessful.String()
+			// execution status is Failed when a task has succeeded condition with status set to false
+			case t.haveAnyRunsFailed():
+				s = v1.TaskRunReasonFailed.String()
+			default:
+				// None includes skipped as well
+				s = PipelineTaskStateNone
+			}
+			tStatus[PipelineTaskStatusPrefix+t.PipelineTask.Name+PipelineTaskStatusSuffix] = s
+		}
+	}
+	return tStatus
+}
+
 // completedOrSkippedTasks returns a list of the names of all of the PipelineTasks in state
 // which have completed or skipped
 func (facts *PipelineRunFacts) completedOrSkippedDAGTasks() []string {

pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go

@@ -2448,6 +2448,149 @@ func TestPipelineRunFacts_GetPipelineTaskStatus(t *testing.T) {
 	}
 }
 
+func TestPipelineRunFacts_GetPipelineFinalTaskStatus(t *testing.T) {
+	tcs := []struct {
+		name           string
+		state          PipelineRunState
+		finalTasks     []v1.PipelineTask
+		expectedStatus map[string]string
+	}{{
+		name:       "no-tasks-started",
+		state:      noneStartedState,
+		finalTasks: []v1.PipelineTask{pts[0], pts[1]},
+		expectedStatus: map[string]string{
+			PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone,
+			PipelineTaskStatusPrefix + pts[1].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone,
+		},
+	}, {
+		name:       "one-task-started",
+		state:      oneStartedState,
+		finalTasks: []v1.PipelineTask{pts[0], pts[1]},
+		expectedStatus: map[string]string{
+			PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone,
+			PipelineTaskStatusPrefix + pts[1].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone,
+		},
+	}, {
+		name:       "one-task-finished",
+		state:      oneFinishedState,
+		finalTasks: []v1.PipelineTask{pts[0], pts[1]},
+		expectedStatus: map[string]string{
+			PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: v1.TaskRunReasonSuccessful.String(),
+			PipelineTaskStatusPrefix + pts[1].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone,
+		},
+	}, {
+		name:       "one-task-failed",
+		state:      oneFailedState,
+		finalTasks: []v1.PipelineTask{pts[0], pts[1]},
+		expectedStatus: map[string]string{
+			PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: v1.TaskRunReasonFailed.String(),
+			PipelineTaskStatusPrefix + pts[1].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone,
+		},
+	}, {
+		name:       "all-finished",
+		state:      allFinishedState,
+		finalTasks: []v1.PipelineTask{pts[0], pts[1]},
+		expectedStatus: map[string]string{
+			PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix: v1.TaskRunReasonSuccessful.String(),
+			PipelineTaskStatusPrefix + pts[1].Name + PipelineTaskStatusSuffix: v1.TaskRunReasonSuccessful.String(),
+		},
+	}, {
+		name: "task-with-when-expressions-passed",
+		state: PipelineRunState{{
+			PipelineTask: &pts[9],
+			TaskRunNames: []string{"pr-guard-succeeded-task-not-started"},
+			TaskRuns:     nil,
+			ResolvedTask: &resources.ResolvedTask{
+				TaskSpec: &task.Spec,
+			},
+		}},
+		finalTasks: []v1.PipelineTask{pts[9]},
+		expectedStatus: map[string]string{
+			PipelineTaskStatusPrefix + pts[9].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone,
+		},
+	}, {
+		name: "tasks-when-expression-failed-and-task-skipped",
+		state: PipelineRunState{{
+			PipelineTask: &pts[10],
+			TaskRunNames: []string{"pr-guardedtask-skipped"},
+			ResolvedTask: &resources.ResolvedTask{
+				TaskSpec: &task.Spec,
+			},
+		}},
+		finalTasks: []v1.PipelineTask{pts[10]},
+		expectedStatus: map[string]string{
+			PipelineTaskStatusPrefix + pts[10].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone,
+		},
+	}, {
+		name: "when-expression-task-with-parent-started",
+		state: PipelineRunState{{
+			PipelineTask: &pts[0],
+			TaskRuns:     []*v1.TaskRun{makeStarted(trs[0])},
+			ResolvedTask: &resources.ResolvedTask{
+				TaskSpec: &task.Spec,
+			},
+		}, {
+			PipelineTask: &pts[11],
+			TaskRuns:     nil,
+			ResolvedTask: &resources.ResolvedTask{
+				TaskSpec: &task.Spec,
+			},
+		}},
+		finalTasks: []v1.PipelineTask{pts[0], pts[11]},
+		expectedStatus: map[string]string{
+			PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix:  PipelineTaskStateNone,
+			PipelineTaskStatusPrefix + pts[11].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone,
+		},
+	}, {
+		name:       "task-cancelled",
+		state:      taskCancelled,
+		finalTasks: []v1.PipelineTask{pts[4]},
+		expectedStatus: map[string]string{
+			PipelineTaskStatusPrefix + pts[4].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone,
+		},
+	}, {
+		name: "one-skipped-one-failed-aggregate-status-must-be-failed",
+		state: PipelineRunState{{
+			PipelineTask: &pts[10],
+			TaskRunNames: []string{"pr-guardedtask-skipped"},
+			ResolvedTask: &resources.ResolvedTask{
+				TaskSpec: &task.Spec,
+			},
+		}, {
+			PipelineTask: &pts[0],
+			TaskRunNames: []string{"pipelinerun-mytask1"},
+			TaskRuns:     []*v1.TaskRun{makeFailed(trs[0])},
+			ResolvedTask: &resources.ResolvedTask{
+				TaskSpec: &task.Spec,
+			},
+		}},
+		finalTasks: []v1.PipelineTask{pts[0], pts[10]},
+		expectedStatus: map[string]string{
+			PipelineTaskStatusPrefix + pts[0].Name + PipelineTaskStatusSuffix:  v1.PipelineRunReasonFailed.String(),
+			PipelineTaskStatusPrefix + pts[10].Name + PipelineTaskStatusSuffix: PipelineTaskStateNone,
+		},
+	}}
+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			d, err := dag.Build(v1.PipelineTaskList(tc.finalTasks), v1.PipelineTaskList(tc.finalTasks).Deps())
+			if err != nil {
+				t.Fatalf("Unexpected error while building graph for DAG tasks %v: %v", tc.finalTasks, err)
+			}
+			facts := PipelineRunFacts{
+				State:           tc.state,
+				FinalTasksGraph: d,
+				TimeoutsState: PipelineRunTimeoutsState{
+					Clock: testClock,
+				},
+			}
+			s := facts.GetPipelineFinalTaskStatus()
+			if d := cmp.Diff(tc.expectedStatus, s); d != "" {
+				t.Fatalf("Test failed: %s Mismatch in pipelineFinalTask execution state %s", tc.name, diff.PrintWantGot(d))
+			}
+		})
+	}
+}
+
 func TestPipelineRunFacts_GetSkippedTasks(t *testing.T) {
 	for _, tc := range []struct {
 		name                 string