From 33d7eacc2bde2bf9197525ba3ecf5ce47a624f43 Mon Sep 17 00:00:00 2001 From: joey Date: Wed, 18 Jan 2023 15:54:42 +0800 Subject: [PATCH] fix task parameter value substitution error due to propagateParams --- pkg/reconciler/pipelinerun/resources/apply.go | 64 +++-- .../pipelinerun/resources/apply_test.go | 254 ++++++++++++++++++ 2 files changed, 295 insertions(+), 23 deletions(-) diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index d52bccf4513..c43ec9e5dbf 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -236,7 +236,7 @@ func ApplyReplacements(p *v1beta1.PipelineSpec, replacements map[string]string, if p.Tasks[i].TaskRef != nil && p.Tasks[i].TaskRef.Params != nil { p.Tasks[i].TaskRef.Params = replaceParamValues(p.Tasks[i].TaskRef.Params, replacements, arrayReplacements, objectReplacements) } - p.Tasks[i], replacements, arrayReplacements, objectReplacements = propagateParams(p.Tasks[i], replacements, arrayReplacements, objectReplacements) + p.Tasks[i] = propagateParams(p.Tasks[i], replacements, arrayReplacements, objectReplacements) } for i := range p.Finally { @@ -251,38 +251,56 @@ func ApplyReplacements(p *v1beta1.PipelineSpec, replacements map[string]string, if p.Finally[i].TaskRef != nil && p.Finally[i].TaskRef.Params != nil { p.Finally[i].TaskRef.Params = replaceParamValues(p.Finally[i].TaskRef.Params, replacements, arrayReplacements, objectReplacements) } - p.Finally[i], replacements, arrayReplacements, objectReplacements = propagateParams(p.Finally[i], replacements, arrayReplacements, objectReplacements) + p.Finally[i] = propagateParams(p.Finally[i], replacements, arrayReplacements, objectReplacements) } return p } -func propagateParams(t v1beta1.PipelineTask, replacements map[string]string, arrayReplacements map[string][]string, objectReplacements map[string]map[string]string) (v1beta1.PipelineTask, map[string]string, map[string][]string, map[string]map[string]string) { - if t.TaskSpec != nil { - // check if there are task parameters defined that match the params at pipeline level - if len(t.Params) > 0 { - for _, par := range t.Params { - for _, pattern := range paramPatterns { - checkName := fmt.Sprintf(pattern, par.Name) - // Scoping. Task Params will replace Pipeline Params - if _, ok := replacements[checkName]; ok { - replacements[checkName] = par.Value.StringVal - } - if _, ok := arrayReplacements[checkName]; ok { - arrayReplacements[checkName] = par.Value.ArrayVal - } - if _, ok := objectReplacements[checkName]; ok { - objectReplacements[checkName] = par.Value.ObjectVal - for k, v := range par.Value.ObjectVal { - replacements[fmt.Sprintf(objectIndividualVariablePattern, par.Name, k)] = v - } +// propagateParams returns a Pipeline Task spec that is the same as the input Pipeline Task spec, but with +// all parameter replacements from `stringReplacements`, `arrayReplacements`, and `objectReplacements` substituted. +// It does not modify `stringReplacements`, `arrayReplacements`, or `objectReplacements`. +func propagateParams(t v1beta1.PipelineTask, stringReplacements map[string]string, arrayReplacements map[string][]string, objectReplacements map[string]map[string]string) v1beta1.PipelineTask { + if t.TaskSpec == nil { + return t + } + // check if there are task parameters defined that match the params at pipeline level + if len(t.Params) > 0 { + stringReplacementsDup := make(map[string]string) + arrayReplacementsDup := make(map[string][]string) + objectReplacementsDup := make(map[string]map[string]string) + for k, v := range stringReplacements { + stringReplacementsDup[k] = v + } + for k, v := range arrayReplacements { + arrayReplacementsDup[k] = v + } + for k, v := range objectReplacements { + objectReplacementsDup[k] = v + } + for _, par := range t.Params { + for _, pattern := range paramPatterns { + checkName := fmt.Sprintf(pattern, par.Name) + // Scoping. Task Params will replace Pipeline Params + if _, ok := stringReplacementsDup[checkName]; ok { + stringReplacementsDup[checkName] = par.Value.StringVal + } + if _, ok := arrayReplacementsDup[checkName]; ok { + arrayReplacementsDup[checkName] = par.Value.ArrayVal + } + if _, ok := objectReplacementsDup[checkName]; ok { + objectReplacementsDup[checkName] = par.Value.ObjectVal + for k, v := range par.Value.ObjectVal { + stringReplacementsDup[fmt.Sprintf(objectIndividualVariablePattern, par.Name, k)] = v } } } } - t.TaskSpec.TaskSpec = *resources.ApplyReplacements(&t.TaskSpec.TaskSpec, replacements, arrayReplacements) + t.TaskSpec.TaskSpec = *resources.ApplyReplacements(&t.TaskSpec.TaskSpec, stringReplacementsDup, arrayReplacementsDup) + } else { + t.TaskSpec.TaskSpec = *resources.ApplyReplacements(&t.TaskSpec.TaskSpec, stringReplacements, arrayReplacements) } - return t, replacements, arrayReplacements, objectReplacements + return t } func replaceParamValues(params []v1beta1.Param, stringReplacements map[string]string, arrayReplacements map[string][]string, objectReplacements map[string]map[string]string) []v1beta1.Param { diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index ab59801d165..af3c0503318 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -1450,6 +1450,260 @@ func TestApplyParameters(t *testing.T) { }}, }, }, + { + name: "tasks with the same parameter name but referencing different values", + original: v1beta1.PipelineSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Default: &v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "a", + }, + Type: v1beta1.ParamTypeString, + }, + }, + Tasks: []v1beta1.PipelineTask{ + { + Name: "previous-task-with-result", + }, + { + Name: "print-msg", + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Type: v1beta1.ParamTypeString, + }, + }, + }, + }, + Params: []v1beta1.Param{ + { + Name: "param1", + Value: v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "$(tasks.previous-task-with-result.results.Output)", + }, + }, + }, + }, + { + Name: "print-msg-2", + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Type: v1beta1.ParamTypeString, + }, + }, + }, + }, + Params: []v1beta1.Param{ + { + Name: "param1", + Value: v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "$(params.param1)", + }, + }, + }, + }, + }, + }, + expected: v1beta1.PipelineSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Default: &v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "a", + }, + Type: v1beta1.ParamTypeString, + }, + }, + Tasks: []v1beta1.PipelineTask{ + { + Name: "previous-task-with-result", + }, + { + Name: "print-msg", + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Type: v1beta1.ParamTypeString, + }, + }, + }, + }, + Params: []v1beta1.Param{ + { + Name: "param1", + Value: v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "$(tasks.previous-task-with-result.results.Output)", + }, + }, + }, + }, + { + Name: "print-msg-2", + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Type: v1beta1.ParamTypeString, + }, + }, + }, + }, + Params: []v1beta1.Param{ + { + Name: "param1", + Value: v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "a", + }, + }, + }, + }, + }, + }, + }, + { + name: "finally tasks with the same parameter name but referencing different values", + original: v1beta1.PipelineSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Default: &v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "a", + }, + Type: v1beta1.ParamTypeString, + }, + }, + Tasks: []v1beta1.PipelineTask{ + { + Name: "previous-task-with-result", + }, + }, + Finally: []v1beta1.PipelineTask{ + { + Name: "print-msg", + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Type: v1beta1.ParamTypeString, + }, + }, + }, + }, + Params: []v1beta1.Param{ + { + Name: "param1", + Value: v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "$(tasks.previous-task-with-result.results.Output)", + }, + }, + }, + }, + { + Name: "print-msg-2", + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Type: v1beta1.ParamTypeString, + }, + }, + }, + }, + Params: []v1beta1.Param{ + { + Name: "param1", + Value: v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "$(params.param1)", + }, + }, + }, + }, + }, + }, + expected: v1beta1.PipelineSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Default: &v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "a", + }, + Type: v1beta1.ParamTypeString, + }, + }, + Tasks: []v1beta1.PipelineTask{ + { + Name: "previous-task-with-result", + }, + }, + Finally: []v1beta1.PipelineTask{ + { + Name: "print-msg", + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Type: v1beta1.ParamTypeString, + }, + }, + }, + }, + Params: []v1beta1.Param{ + { + Name: "param1", + Value: v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "$(tasks.previous-task-with-result.results.Output)", + }, + }, + }, + }, + { + Name: "print-msg-2", + TaskSpec: &v1beta1.EmbeddedTask{ + TaskSpec: v1beta1.TaskSpec{ + Params: []v1beta1.ParamSpec{ + { + Name: "param1", + Type: v1beta1.ParamTypeString, + }, + }, + }, + }, + Params: []v1beta1.Param{ + { + Name: "param1", + Value: v1beta1.ParamValue{ + Type: v1beta1.ParamTypeString, + StringVal: "a", + }, + }, + }, + }, + }, + }, + }, } { tt := tt // capture range variable ctx := context.Background()