Skip to content

Commit

Permalink
Implicit params: don't apply PipelineSpec params to TaskRefs.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@wlynch @tekton-robot
wlynch authored and tekton-robot committed Jan 27, 2022
1 parent bac011c commit bf0bf87
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 37 deletions.
19 changes: 11 additions & 8 deletions pkg/apis/pipeline/v1beta1/pipeline_defaults.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
128 changes: 99 additions & 29 deletions pkg/apis/pipeline/v1beta1/pipelinerun_defaults_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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",
Expand All @@ -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))
}
})
Expand Down

0 comments on commit bf0bf87

Please sign in to comment.