From edf70b37a4c2634f65c0a16a045c6d35ef35163c Mon Sep 17 00:00:00 2001 From: Billy Lynch Date: Thu, 13 Jan 2022 17:58:17 -0500 Subject: [PATCH] Implicit params: don't apply PipelineSpec params to TaskRefs. Previously we were applying the params to all PipelineTasks, including refs. This is incorrect behavior since the parameters should only be propagated to things within the same document. --- .../pipeline/v1beta1/pipeline_defaults.go | 19 +-- .../v1beta1/pipelinerun_defaults_test.go | 128 ++++++++++++++---- 2 files changed, 110 insertions(+), 37 deletions(-) diff --git a/pkg/apis/pipeline/v1beta1/pipeline_defaults.go b/pkg/apis/pipeline/v1beta1/pipeline_defaults.go index 32e60fbfdcd..8d77712f8b1 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_defaults.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_defaults.go @@ -41,32 +41,35 @@ func (ps *PipelineSpec) SetDefaults(ctx context.Context) { } for i, pt := range ps.Tasks { ctx := ctx // Ensure local scoping per Task - if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" { - ctx = addContextParams(ctx, pt.Params) - ps.Tasks[i].Params = getContextParams(ctx, pt.Params...) - } + if pt.TaskRef != nil { if pt.TaskRef.Kind == "" { pt.TaskRef.Kind = NamespacedTaskKind } } if pt.TaskSpec != nil { + // Only propagate param context to the spec - ref params should + // still be explicitly set. + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" { + ctx = addContextParams(ctx, pt.Params) + ps.Tasks[i].Params = getContextParams(ctx, pt.Params...) + } pt.TaskSpec.SetDefaults(ctx) } } for i, ft := range ps.Finally { ctx := ctx // Ensure local scoping per Task - if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" { - ctx = addContextParams(ctx, ft.Params) - ps.Finally[i].Params = getContextParams(ctx, ft.Params...) - } if ft.TaskRef != nil { if ft.TaskRef.Kind == "" { ft.TaskRef.Kind = NamespacedTaskKind } } if ft.TaskSpec != nil { + if config.FromContextOrDefaults(ctx).FeatureFlags.EnableAPIFields == "alpha" { + ctx = addContextParams(ctx, ft.Params) + ps.Finally[i].Params = getContextParams(ctx, ft.Params...) + } ft.TaskSpec.SetDefaults(ctx) } } diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go index 85e241b7fe4..3daac7b6b49 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go @@ -107,11 +107,30 @@ func TestPipelineRunSpec_SetDefaults(t *testing.T) { }, }, PipelineSpec: &v1beta1.PipelineSpec{ - Tasks: []v1beta1.PipelineTask{{ - TaskSpec: &v1beta1.EmbeddedTask{ - TaskSpec: v1beta1.TaskSpec{}, + Tasks: []v1beta1.PipelineTask{ + { + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{}, + }, }, - }}, + { + TaskRef: &v1beta1.TaskRef{ + Name: "baz", + }, + }, + }, + Finally: []v1beta1.PipelineTask{ + { + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{}, + }, + }, + { + TaskRef: &v1beta1.TaskRef{ + Name: "baz", + }, + }, + }, }, }, want: &v1beta1.PipelineRunSpec{ @@ -132,38 +151,86 @@ func TestPipelineRunSpec_SetDefaults(t *testing.T) { }, }, PipelineSpec: &v1beta1.PipelineSpec{ - Tasks: []v1beta1.PipelineTask{{ - TaskSpec: &v1beta1.EmbeddedTask{ - TaskSpec: v1beta1.TaskSpec{ - Params: []v1beta1.ParamSpec{ - { - Name: "foo", - Type: v1beta1.ParamTypeString, + Tasks: []v1beta1.PipelineTask{ + { + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "foo", + Type: v1beta1.ParamTypeString, + }, + { + Name: "bar", + Type: v1beta1.ParamTypeArray, + }, }, - { - Name: "bar", - Type: v1beta1.ParamTypeArray, + }, + }, + Params: []v1beta1.Param{ + { + Name: "foo", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(params.foo)", + }, + }, + { + Name: "bar", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeArray, + ArrayVal: []string{"$(params.bar[*])"}, }, }, }, }, - Params: []v1beta1.Param{ - { - Name: "foo", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeString, - StringVal: "$(params.foo)", + { + TaskRef: &v1beta1.TaskRef{ + Name: "baz", + Kind: v1beta1.NamespacedTaskKind, + }, + }, + }, + Finally: []v1beta1.PipelineTask{ + { + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "foo", + Type: v1beta1.ParamTypeString, + }, + { + Name: "bar", + Type: v1beta1.ParamTypeArray, + }, + }, }, }, - { - Name: "bar", - Value: v1beta1.ArrayOrString{ - Type: v1beta1.ParamTypeArray, - ArrayVal: []string{"$(params.bar[*])"}, + Params: []v1beta1.Param{ + { + Name: "foo", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeString, + StringVal: "$(params.foo)", + }, + }, + { + Name: "bar", + Value: v1beta1.ArrayOrString{ + Type: v1beta1.ParamTypeArray, + ArrayVal: []string{"$(params.bar[*])"}, + }, }, }, }, - }}, + { + TaskRef: &v1beta1.TaskRef{ + Name: "baz", + Kind: v1beta1.NamespacedTaskKind, + }, + }, + }, Params: []v1beta1.ParamSpec{ { Name: "foo", @@ -186,13 +253,16 @@ func TestPipelineRunSpec_SetDefaults(t *testing.T) { } tc.prs.SetDefaults(ctx) - sortPS := func(x, y v1beta1.ParamSpec) bool { + sortParamSpecs := func(x, y v1beta1.ParamSpec) bool { + return x.Name < y.Name + } + sortParams := func(x, y v1beta1.Param) bool { return x.Name < y.Name } - sortP := func(x, y v1beta1.Param) bool { + sortTasks := func(x, y v1beta1.PipelineTask) bool { return x.Name < y.Name } - if d := cmp.Diff(tc.want, tc.prs, cmpopts.SortSlices(sortPS), cmpopts.SortSlices(sortP)); d != "" { + if d := cmp.Diff(tc.want, tc.prs, cmpopts.SortSlices(sortParamSpecs), cmpopts.SortSlices(sortParams), cmpopts.SortSlices(sortTasks)); d != "" { t.Errorf("Mismatch of PipelineRunSpec %s", diff.PrintWantGot(d)) } })