From f1652379cdd89fa0cd2bbf3430fbdfd7daf61a2a Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Mon, 20 Mar 2023 15:28:59 +0000 Subject: [PATCH] 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 yongxuanzhang@google.com --- .../v1beta1/pipelineruns/6139-regression.yaml | 45 ++++++++++++++++ pkg/reconciler/pipelinerun/pipelinerun.go | 2 +- .../pipelinerun/pipelinerun_test.go | 2 +- pkg/reconciler/pipelinerun/resources/apply.go | 26 +++++---- .../pipelinerun/resources/apply_test.go | 54 ++++++++++++++----- 5 files changed, 103 insertions(+), 26 deletions(-) create mode 100644 examples/v1beta1/pipelineruns/6139-regression.yaml 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 83f507b738b..561fd7733b3 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/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 diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 236fbc6b011..a358e3ae497 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/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 diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index 08e8ff8f097..08a4e3dd3c3 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/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 diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index 8aa34e2979f..6a14d60e1ad 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/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 != "" {