don't return validation error when taskrun failed/skipped

This commit aims to fix #6383, #6139 and supports #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 [email protected]

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

@@ -687,7 +687,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/resources/apply.go

@@ -338,13 +338,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{}
@@ -366,11 +362,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)
@@ -406,6 +398,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
@@ -423,6 +422,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

@@ -3512,6 +3512,7 @@ func TestApplyTaskResultsToPipelineResults_Success(t *testing.T) {
 		results         []v1beta1.PipelineResult
 		taskResults     map[string][]v1beta1.TaskRunResult
 		runResults      map[string][]v1beta1.CustomRunResult
+		taskstatus      map[string]string
 		skippedTasks    []v1beta1.SkippedTask
 		expectedResults []v1beta1.PipelineRunResult
 	}{{
@@ -3789,13 +3790,47 @@ func TestApplyTaskResultsToPipelineResults_Success(t *testing.T) {
 				Value: *v1beta1.NewStructuredValues("rae"),
 			}},
 		},
-		skippedTasks: []v1beta1.SkippedTask{{
-			Name: "skippedTask",
+		taskstatus:      map[string]string{resources.PipelineTaskStatusPrefix + "skippedTask" + resources.PipelineTaskStatusSuffix: resources.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{resources.PipelineTaskStatusPrefix + "pt1" + resources.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{resources.PipelineTaskStatusPrefix + "pt1" + resources.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{resources.PipelineTaskStatusPrefix + "pt1" + resources.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 := resources.ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.skippedTasks)
+			received, err := resources.ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.taskstatus)
 			if err != nil {
 				t.Errorf("Got unecpected error:%v", err)
 			}
@@ -4014,6 +4049,7 @@ func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) {
 			received, err := resources.ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, nil /*skipped tasks*/)
 			if err == nil {
 				t.Errorf("Expect error but got nil")
+				return
 			}
 
 			if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" {

test/pipelinerun_test.go

@@ -744,7 +744,7 @@ metadata:
 spec:
   params:
   - name: HELLO
-    value: "Hello World!" 
+    value: "Hello World!"
   pipelineRef:
     name: %s
 `, namespace, pipelineName))
@@ -952,3 +952,87 @@ func getLimitRange(name, namespace, resourceCPU, resourceMemory, resourceEphemer
 		},
 	}
 }
+
+func TestPipelineRunTaskFailed(t *testing.T) {
+	ctx := context.Background()
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
+	c, namespace := setup(ctx, t)
+
+	knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf)
+	defer tearDown(ctx, t, c, namespace)
+
+	taskName := helpers.ObjectNameForTest(t)
+	pipelineName := helpers.ObjectNameForTest(t)
+	prName := helpers.ObjectNameForTest(t)
+
+	t.Logf("Creating Task, Pipeline, and Pending PipelineRun %s in namespace %s", prName, namespace)
+
+	if _, err := c.V1beta1TaskClient.Create(ctx, parse.MustParseV1beta1Task(t, fmt.Sprintf(`
+metadata:
+  name: %s
+  namespace: %s
+spec:
+  steps:
+  - image: ubuntu
+    command: ['/bin/bash']
+    args: ['-c', 'echo hello, world']
+`, taskName, namespace)), metav1.CreateOptions{}); err != nil {
+		t.Fatalf("Failed to create Task `%s`: %s", taskName, err)
+	}
+
+	if _, err := c.V1beta1PipelineClient.Create(ctx, parse.MustParseV1beta1Pipeline(t, fmt.Sprintf(`
+metadata:
+  name: %s
+  namespace: %s
+spec:
+  tasks:
+  - name: task
+    taskRef:
+      name: %s
+`, pipelineName, namespace, taskName)), metav1.CreateOptions{}); err != nil {
+		t.Fatalf("Failed to create Pipeline `%s`: %s", pipelineName, err)
+	}
+
+	pipelineRun, err := c.V1beta1PipelineRunClient.Create(ctx, parse.MustParseV1beta1PipelineRun(t, fmt.Sprintf(`
+metadata:
+  name: %s
+  namespace: %s
+spec:
+  pipelineSpec:
+    results:
+      - name: abc
+        value: "$(tasks.xxx.results.abc)"
+    tasks:
+    - name: xxx
+      taskSpec:
+        results:
+          - name: abc
+        steps:
+        - name: update-sa
+          image: bash:latest
+          script: |
+            echo 'test' >  $(results.abc.path)
+            exit 1
+`, prName, namespace)), metav1.CreateOptions{})
+	if err != nil {
+		t.Fatalf("Failed to create PipelineRun `%s`: %s", prName, err)
+	}
+	// Wait for the PipelineRun to fail.
+	if err := WaitForPipelineRunState(ctx, c, prName, timeout, PipelineRunFailed(prName), "PipelineRunFailed", v1beta1Version); err != nil {
+		t.Fatalf("Waiting for PipelineRun to fail: %v", err)
+	}
+
+	pipelineRun, err = c.V1beta1PipelineRunClient.Get(ctx, prName, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Error getting PipelineRun %s: %s", prName, err)
+	}
+
+	if pipelineRun.Status.GetCondition(apis.ConditionSucceeded).IsTrue() {
+		t.Errorf("Expected PipelineRun to fail but found condition: %s", pipelineRun.Status.GetCondition(apis.ConditionSucceeded))
+	}
+	expectedMessage := "Tasks Completed: 1 (Failed: 1, Cancelled 0), Skipped: 0"
+	if pipelineRun.Status.GetCondition(apis.ConditionSucceeded).Message != expectedMessage {
+		t.Errorf("Expected PipelineRun to fail with condition message: %s but got: %s", expectedMessage, pipelineRun.Status.GetCondition(apis.ConditionSucceeded).Message)
+	}
+}