From 81b066becc84ba589744599e018a21898a7e9fc3 Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Mon, 13 Feb 2023 19:20:13 +0000 Subject: [PATCH] don't validate skipped task results for pipeline results This commit skip the validation for skipped task results in pipeline results. This fixes #6139. The skipped task results are not emitted by design thus we shouldn't validate if they are emitted. Pipeline results referenced to skipped task will not be emitted. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- docs/pipelines.md | 3 ++- pkg/reconciler/pipelinerun/pipelinerun.go | 4 ++-- pkg/reconciler/pipelinerun/resources/apply.go | 14 ++++++++++- .../pipelinerun/resources/apply_test.go | 24 +++++++++++++++++-- 4 files changed, 39 insertions(+), 6 deletions(-) diff --git a/docs/pipelines.md b/docs/pipelines.md index 0ad7782e082..aa1741c1769 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -1037,6 +1037,7 @@ This will cause an `InvalidTaskResultReference` validation error during `Pipelin **Note:** Since a `Pipeline Result` can contain references to multiple `Task Results`, if any of those `Task Result` references are invalid the entire `Pipeline Result` is not emitted. +**Note:** If a `PipelineTask` referenced by the `Pipeline Result` was skipped, the `Pipeline Result` will not be emitted and the `PipelineRun` will not fail due to a missing result. ## Configuring the `Task` execution order @@ -1279,7 +1280,7 @@ results: value: $(finally.check-count.results.comment-count-validate) finally: - name: check-count - taskRef: + taskRef: name: example-task-name ``` diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 7a6e7357a09..8fd0531e80c 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -703,8 +703,8 @@ 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(pipelineSpec.Results, - pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults()) + pr.Status.PipelineResults, err = resources.ApplyTaskResultsToPipelineResults(ctx, pipelineSpec.Results, + pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pr.Status.SkippedTasks) 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 7c1d75eccab..5cbfec61299 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -317,11 +317,18 @@ func replaceParamValues(params []v1beta1.Param, stringReplacements map[string]st // and omitted from the returned slice. A nil slice is returned if no results are passed in or all // results are invalid. func ApplyTaskResultsToPipelineResults( + ctx context.Context, results []v1beta1.PipelineResult, taskRunResults map[string][]v1beta1.TaskRunResult, - customTaskResults map[string][]v1alpha1.RunResult) ([]v1beta1.PipelineRunResult, error) { + customTaskResults map[string][]v1alpha1.RunResult, + skippedTasks []v1beta1.SkippedTask) ([]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{} objectReplacements := map[string]map[string]string{} @@ -342,6 +349,11 @@ 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) diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index a43e0e540ed..9c13f794187 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -3132,7 +3132,7 @@ func TestApplyFinallyResultsToPipelineResults(t *testing.T) { }, } { t.Run(tc.description, func(t *testing.T) { - received, _ := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults) + received, _ := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, nil /* skippedTasks */) if d := cmp.Diff(tc.expected, received); d != "" { t.Errorf(diff.PrintWantGot(d)) } @@ -3146,6 +3146,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { results []v1beta1.PipelineResult taskResults map[string][]v1beta1.TaskRunResult runResults map[string][]v1alpha1.RunResult + skippedTasks []v1beta1.SkippedTask expectedResults []v1beta1.PipelineRunResult expectedError error }{{ @@ -3600,9 +3601,28 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { Name: "pipeline-result-2", Value: *v1beta1.NewStructuredValues("do, rae, mi, rae, do"), }}, + }, { + description: "multiple-results-skipped-and-normal-tasks", + results: []v1beta1.PipelineResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo)"), + }, { + Name: "pipeline-result-2", + Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo), $(tasks.normaltask.results.baz)"), + }}, + taskResults: map[string][]v1beta1.TaskRunResult{ + "normaltask": {{ + Name: "baz", + Value: *v1beta1.NewStructuredValues("rae"), + }}, + }, + skippedTasks: []v1beta1.SkippedTask{{ + Name: "skippedTask", + }}, + expectedResults: nil, }} { t.Run(tc.description, func(t *testing.T) { - received, err := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults) + received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.skippedTasks) if tc.expectedError != nil { if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" { t.Errorf("ApplyTaskResultsToPipelineResults() errors diff %s", diff.PrintWantGot(d))