From da026702e6031f11370f45047e91618f69d6a18e 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. --- docs/pipelines.md | 7 +++--- .../clientset/versioned/fake/register.go | 14 +++++------ .../clientset/versioned/scheme/register.go | 14 +++++------ .../clientset/versioned/fake/register.go | 14 +++++------ .../clientset/versioned/scheme/register.go | 14 +++++------ .../clientset/versioned/fake/register.go | 14 +++++------ .../clientset/versioned/scheme/register.go | 14 +++++------ pkg/reconciler/pipelinerun/pipelinerun.go | 4 ++-- pkg/reconciler/pipelinerun/resources/apply.go | 14 ++++++++++- .../pipelinerun/resources/apply_test.go | 24 +++++++++++++++++-- 10 files changed, 83 insertions(+), 50 deletions(-) diff --git a/docs/pipelines.md b/docs/pipelines.md index 868c7e8aa98..0fcba95f0c8 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 not be emitted and the `PipelineRun` will not fail due to a missing result. ## 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/client/clientset/versioned/fake/register.go b/pkg/client/clientset/versioned/fake/register.go index 951cec59e8f..27907d4e65f 100644 --- a/pkg/client/clientset/versioned/fake/register.go +++ b/pkg/client/clientset/versioned/fake/register.go @@ -41,14 +41,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/pkg/client/clientset/versioned/scheme/register.go b/pkg/client/clientset/versioned/scheme/register.go index 21c3e532dfd..ffc8f29c1f7 100644 --- a/pkg/client/clientset/versioned/scheme/register.go +++ b/pkg/client/clientset/versioned/scheme/register.go @@ -41,14 +41,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/pkg/client/resolution/clientset/versioned/fake/register.go b/pkg/client/resolution/clientset/versioned/fake/register.go index 70d792b35d6..8469024ac97 100644 --- a/pkg/client/resolution/clientset/versioned/fake/register.go +++ b/pkg/client/resolution/clientset/versioned/fake/register.go @@ -39,14 +39,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/pkg/client/resolution/clientset/versioned/scheme/register.go b/pkg/client/resolution/clientset/versioned/scheme/register.go index 9fe0d86a62e..19e5de5b4c6 100644 --- a/pkg/client/resolution/clientset/versioned/scheme/register.go +++ b/pkg/client/resolution/clientset/versioned/scheme/register.go @@ -39,14 +39,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/pkg/client/resource/clientset/versioned/fake/register.go b/pkg/client/resource/clientset/versioned/fake/register.go index 78bbf3de2ae..e33dea7aa03 100644 --- a/pkg/client/resource/clientset/versioned/fake/register.go +++ b/pkg/client/resource/clientset/versioned/fake/register.go @@ -37,14 +37,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/pkg/client/resource/clientset/versioned/scheme/register.go b/pkg/client/resource/clientset/versioned/scheme/register.go index 2eb6b537e5a..972999462fa 100644 --- a/pkg/client/resource/clientset/versioned/scheme/register.go +++ b/pkg/client/resource/clientset/versioned/scheme/register.go @@ -37,14 +37,14 @@ var localSchemeBuilder = runtime.SchemeBuilder{ // AddToScheme adds all types of this clientset into the given scheme. This allows composition // of clientsets, like in: // -// import ( -// "k8s.io/client-go/kubernetes" -// clientsetscheme "k8s.io/client-go/kubernetes/scheme" -// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" -// ) +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) // -// kclientset, _ := kubernetes.NewForConfig(c) -// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) // // After this, RawExtensions in Kubernetes types will serialize kube-aggregator types // correctly. diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 8ff1e1046b9..c5157502014 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -719,8 +719,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 f262a768b9f..1e29273a66a 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -314,11 +314,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][]v1beta1.CustomRunResult) ([]v1beta1.PipelineRunResult, error) { + customTaskResults map[string][]v1beta1.CustomRunResult, + 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{} @@ -339,6 +346,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 0bdaa3c830a..45389c62204 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -3131,7 +3131,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)) } @@ -3145,6 +3145,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 +3600,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))