From 72a556d224ecedcd0a950160fecfa2bd096eaf29 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 remain as the original unsubstituted references. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- docs/pipelines.md | 7 +++-- pkg/reconciler/pipelinerun/pipelinerun.go | 2 +- pkg/reconciler/pipelinerun/resources/apply.go | 16 +++++++++- .../pipelinerun/resources/apply_test.go | 31 +++++++++++++++++-- 4 files changed, 49 insertions(+), 7 deletions(-) diff --git a/docs/pipelines.md b/docs/pipelines.md index a2f3b89ab5d..d4797b01aa6 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -336,7 +336,7 @@ tasks: value: https://github.com/tektoncd/catalog.git - name: revision # value can use params declared at the pipeline level or a static value like main - value: $(params.gitRevision) + value: $(params.gitRevision) - name: pathInRepo value: task/golang-build/0.3/golang-build.yaml ``` @@ -1079,6 +1079,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 remain as the original unsubstituted references. ## Configuring the `Task` execution order @@ -1322,7 +1323,7 @@ results: value: $(finally.check-count.results.comment-count-validate) finally: - name: check-count - taskRef: + taskRef: name: example-task-name ``` @@ -1810,7 +1811,7 @@ Consult the documentation of the custom task that you are using to determine whe Pipelines do not support the following items with custom tasks: * Pipeline Resources -### Known Custom Tasks +### Known Custom Tasks We try to list as many known Custom Tasks as possible here so that users can easily find what they want. Please feel free to share the Custom Task you implemented in this table. diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index a84ca645fc1..088256256cb 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -723,7 +723,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(pipelineSpec.Results, - pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults()) + pipelineRunFacts.State.GetTaskRunsResults(), pipelineRunFacts.State.GetRunsResults(), pr.Status.SkippedTasks, logger) 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 470485a30fc..00b0d5d12d9 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -23,6 +23,8 @@ import ( "strings" "github.com/tektoncd/pipeline/pkg/apis/config" + "go.uber.org/zap" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" "github.com/tektoncd/pipeline/pkg/substitution" @@ -315,9 +317,16 @@ func replaceParamValues(params []v1beta1.Param, stringReplacements map[string]st func ApplyTaskResultsToPipelineResults( results []v1beta1.PipelineResult, taskRunResults map[string][]v1beta1.TaskRunResult, - customTaskResults map[string][]v1beta1.CustomRunResult) ([]v1beta1.PipelineRunResult, error) { + customTaskResults map[string][]v1beta1.CustomRunResult, + skippedTasks []v1beta1.SkippedTask, + logger *zap.SugaredLogger) ([]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{} @@ -338,6 +347,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 { + logger.Debugf("Result reference %s is not substituted because referenced task %s is skipped", variable, variableParts[1]) + 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 0bdaa3c830a..b6ca3729280 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -27,6 +27,7 @@ import ( "github.com/tektoncd/pipeline/test/diff" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/selection" + logtesting "knative.dev/pkg/logging/testing" ) func TestApplyParameters(t *testing.T) { @@ -3131,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(tc.results, tc.taskResults, tc.runResults, nil, logtesting.TestLogger(t)) if d := cmp.Diff(tc.expected, received); d != "" { t.Errorf(diff.PrintWantGot(d)) } @@ -3145,6 +3146,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { results []v1beta1.PipelineResult taskResults map[string][]v1beta1.TaskRunResult runResults map[string][]v1beta1.CustomRunResult + skippedTasks []v1beta1.SkippedTask expectedResults []v1beta1.PipelineRunResult expectedError error }{{ @@ -3599,9 +3601,34 @@ 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: []v1beta1.PipelineRunResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo)"), + }, { + Name: "pipeline-result-2", + Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo), rae"), + }}, }} { t.Run(tc.description, func(t *testing.T) { - received, err := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults) + received, err := ApplyTaskResultsToPipelineResults(tc.results, tc.taskResults, tc.runResults, tc.skippedTasks, logtesting.TestLogger(t)) if tc.expectedError != nil { if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" { t.Errorf("ApplyTaskResultsToPipelineResults() errors diff %s", diff.PrintWantGot(d))