diff --git a/examples/v1beta1/pipelineruns/6139-regression.yaml b/examples/v1beta1/pipelineruns/6139-regression.yaml new file mode 100644 index 00000000000..f2a73f03299 --- /dev/null +++ b/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)' diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 80e550dd63b..14a0b2db9e1 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/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 diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index edcec080901..8dd02c04804 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/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 diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index 45521ec70fd..a350d98d18e 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/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 != "" { diff --git a/test/pipelinerun_test.go b/test/pipelinerun_test.go index a83cd6b7b34..2875ce90a00 100644 --- a/test/pipelinerun_test.go +++ b/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) + } +}