don't return validation error when taskrun failed/skipped

This commit aims to fix tektoncd#6383, tektoncd#6139 and supports tektoncd#3749. This commits
skip the validation if the taskrun is not successful (e.g. failed or skipped) and
omit the pipelinerun coresponding result. This way the skipped taskrun
won't get validation error for pipelinerun. And the pipelinerun error
won't be overwritten by the validation error.

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

Author: Yongxuanzhang

examples/v1beta1/pipelineruns/6139-regression.yaml

@@ -0,0 +1,45 @@
+apiVersion: tekton.dev/v1beta1
+kind: PipelineRun
+metadata:
+  name: pipelinerun-test
+spec:
+  pipelineSpec:
+    params:
+      - name: say-hello
+        default: 'false'
+    tasks:
+      - name: hello
+        taskSpec:
+          results:
+            - name: result-one
+          steps:
+          - image: alpine
+            script: |
+              #!/bin/sh
+              echo "Hello world!"
+              echo -n "RES1" > $(results.result-one.path)
+
+      - name: goodbye
+        runAfter:
+          - hello
+        taskSpec:
+          results:
+            - name: result-two
+          steps:
+          - image: alpine
+            script: |
+              #!/bin/sh
+              echo "Goodbye world!"
+              echo -n "RES2" > $(results.result-two.path)
+        when:
+          - input: $(params.say-hello)
+            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)'

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -688,7 +688,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
 	pr.Status.SkippedTasks = pipelineRunFacts.GetSkippedTasks()
 	if after.Status == corev1.ConditionTrue || after.Status == corev1.ConditionFalse {
 		pr.Status.PipelineResults, err = resources.ApplyTaskResultsToPipelineResults(ctx, pipelineSpec.Results,
-			pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pr.Status.SkippedTasks)
+			pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pipelineRunFacts.GetPipelineTaskStatus())
 		if err != nil {
 			pr.Status.MarkFailed(ReasonFailedValidation, err.Error())
 			return err

pkg/reconciler/pipelinerun/pipelinerun_test.go

@@ -2511,7 +2511,7 @@ status:
 		mustParseTaskRunWithObjectMeta(t, taskRunObjectMeta("test-pipeline-run-with-timeout-finaltask-1", "foo", "test-pipeline-run-with-timeout",
 			"test-pipeline", "finaltask-1", false), `
 spec:
-  
+
   serviceAccountName: test-sa
   taskRef:
     name: hello-world

pkg/reconciler/pipelinerun/resources/apply.go

@@ -334,13 +334,9 @@ func ApplyTaskResultsToPipelineResults(
 	results []v1beta1.PipelineResult,
 	taskRunResults map[string][]v1beta1.TaskRunResult,
 	customTaskResults map[string][]v1beta1.CustomRunResult,
-	skippedTasks []v1beta1.SkippedTask) ([]v1beta1.PipelineRunResult, error) {
+	taskstatus map[string]string) ([]v1beta1.PipelineRunResult, error) {
 	var runResults []v1beta1.PipelineRunResult
 	var invalidPipelineResults []string
-	skippedTaskNames := map[string]bool{}
-	for _, t := range skippedTasks {
-		skippedTaskNames[t.Name] = true
-	}
 
 	stringReplacements := map[string]string{}
 	arrayReplacements := map[string][]string{}
@@ -362,11 +358,7 @@ func ApplyTaskResultsToPipelineResults(
 				continue
 			}
 			variableParts := strings.Split(variable, ".")
-			// if the referenced task is skipped, we should also skip the results replacements
-			if _, ok := skippedTaskNames[variableParts[1]]; ok {
-				validPipelineResult = false
-				continue
-			}
+
 			if (variableParts[0] != v1beta1.ResultTaskPart && variableParts[0] != v1beta1.ResultFinallyPart) || variableParts[2] != v1beta1.ResultResultPart {
 				validPipelineResult = false
 				invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)
@@ -402,6 +394,13 @@ func ApplyTaskResultsToPipelineResults(
 				} else if resultValue := runResultValue(taskName, resultName, customTaskResults); resultValue != nil {
 					stringReplacements[variable] = *resultValue
 				} else {
+					// if the task is not successful (e.g. skipped or failed) and the results is missing, don't return error
+					if status, ok := taskstatus[PipelineTaskStatusPrefix+taskName+PipelineTaskStatusSuffix]; ok {
+						if status != v1beta1.TaskRunReasonSuccessful.String() {
+							validPipelineResult = false
+							continue
+						}
+					}
 					// referred result name is not existent
 					invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)
 					validPipelineResult = false
@@ -419,6 +418,13 @@ func ApplyTaskResultsToPipelineResults(
 						validPipelineResult = false
 					}
 				} else {
+					// if the task is not successful (e.g. skipped or failed) and the results is missing, don't return error
+					if status, ok := taskstatus[PipelineTaskStatusPrefix+taskName+PipelineTaskStatusSuffix]; ok {
+						if status != v1beta1.TaskRunReasonSuccessful.String() {
+							validPipelineResult = false
+							continue
+						}
+					}
 					// referred result name is not existent
 					invalidPipelineResults = append(invalidPipelineResults, pipelineResult.Name)
 					validPipelineResult = false

pkg/reconciler/pipelinerun/resources/apply_test.go

@@ -3511,7 +3511,7 @@ func TestApplyTaskResultsToPipelineResults_Success(t *testing.T) {
 		results         []v1beta1.PipelineResult
 		taskResults     map[string][]v1beta1.TaskRunResult
 		runResults      map[string][]v1beta1.CustomRunResult
-		skippedTasks    []v1beta1.SkippedTask
+		taskstatus      map[string]string
 		expectedResults []v1beta1.PipelineRunResult
 	}{{
 		description: "non-reference-results",
@@ -3788,13 +3788,47 @@ func TestApplyTaskResultsToPipelineResults_Success(t *testing.T) {
 				Value: *v1beta1.NewStructuredValues("rae"),
 			}},
 		},
-		skippedTasks: []v1beta1.SkippedTask{{
-			Name: "skippedTask",
+		taskstatus:      map[string]string{PipelineTaskStatusPrefix + "skippedTask" + PipelineTaskStatusSuffix: PipelineTaskStateNone},
+		expectedResults: nil,
+	}, {
+		description: "unsuccessful-taskrun-no-results",
+		results: []v1beta1.PipelineResult{{
+			Name:  "foo",
+			Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo)"),
 		}},
+		taskResults:     map[string][]v1beta1.TaskRunResult{},
+		taskstatus:      map[string]string{PipelineTaskStatusPrefix + "pt1" + PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()},
 		expectedResults: nil,
+	}, {
+		description: "unsuccessful-taskrun-no-returned-result-object-ref",
+		results: []v1beta1.PipelineResult{{
+			Name:  "foo",
+			Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo.key1)"),
+		}},
+		taskResults:     map[string][]v1beta1.TaskRunResult{},
+		taskstatus:      map[string]string{PipelineTaskStatusPrefix + "pt1" + PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()},
+		expectedResults: nil,
+	}, {
+		description: "unsuccessful-taskrun-with-results",
+		results: []v1beta1.PipelineResult{{
+			Name:  "foo",
+			Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo[*])"),
+		}},
+		taskResults: map[string][]v1beta1.TaskRunResult{
+			"pt1": {
+				{
+					Name:  "foo",
+					Value: *v1beta1.NewStructuredValues("do", "rae", "mi"),
+				},
+			}},
+		taskstatus: map[string]string{PipelineTaskStatusPrefix + "pt1" + PipelineTaskStatusSuffix: v1beta1.TaskRunReasonFailed.String()},
+		expectedResults: []v1beta1.PipelineRunResult{{
+			Name:  "foo",
+			Value: *v1beta1.NewStructuredValues("do", "rae", "mi"),
+		}},
 	}} {
 		t.Run(tc.description, func(t *testing.T) {
-			received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.skippedTasks)
+			received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.taskstatus)
 			if err != nil {
 				t.Errorf("Got unecpected error:%v", err)
 			}
@@ -3920,15 +3954,6 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) {
 		},
 		expectedResults: nil,
 		expectedError:   fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"),
-	}, {
-		description: "unsuccessful-taskrun-no-returned-result",
-		results: []v1beta1.PipelineResult{{
-			Name:  "foo",
-			Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo)"),
-		}},
-		taskResults:     map[string][]v1beta1.TaskRunResult{},
-		expectedResults: nil,
-		expectedError:   fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"),
 	}, {
 		description: "mixed-success-tasks-some-returned-results",
 		results: []v1beta1.PipelineResult{{
@@ -4010,9 +4035,10 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) {
 		expectedError:   fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"),
 	}} {
 		t.Run(tc.description, func(t *testing.T) {
-			received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, nil /*skipped tasks*/)
+			received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, nil /*tasks status*/)
 			if err == nil {
 				t.Errorf("Expect error but got nil")
+				return
 			}
 
 			if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" {