[release-v0.47.x] don't return validation error when taskrun failed/skipped

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