fix(controller): allow to use global params with withParam #2757

dtaniwaki · 2020-04-20T12:50:16Z

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.

(b)

The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed the CLA.
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

simster7 · 2020-04-20T17:25:36Z

workflow/controller/dag.go

@@ -526,7 +526,12 @@ func (woc *wfOperationCtx) resolveDependencyReferences(dagCtx *dagContext, task
 return nil, errors.InternalWrapError(err)
 }
 fstTmpl := fasttemplate.New(string(taskBytes), "{{", "}}")
- newTaskStr, err := common.Replace(fstTmpl, scope.replaceMap(), true)
+
+ replaceMap := scope.replaceMap()


I think something like

Suggested change

replaceMap := scope.replaceMap()

replaceMap := scope.replaceMap(woc.globalParams)

Would be cleaner.

You could edit the replaceMap function to include this optional parameter and add its contents to the scope if it's not nil

What do you think?

I think the method name replaceMap is not intuitive to accept an argument of params extension. Maybe, we can have something like mergeParams utility method in the common package.

If we want to introduce mergeParams utility method, we should make another PR to replace other codes meging params.

We could change the name to something like prepareScope, here is an example where I do something similar:

https://github.com/argoproj/argo/blob/7cb2fd17765aad691eda25ca4c5acecb89f84394/workflow/controller/steps.go#L466-L471

This is minor though, I'll leave it up to you.

As I found there're many cases of merging params, I defined Params type which has Clone and Merge functions so we can replace those codes with one line.

I also found the order of merging was not correct in my original commit of this PR. scoep.replaceMap() should be merged into globalParmas but not the opposite.

…resolution-with-with-param

simster7 · 2020-04-21T16:42:54Z

workflow/controller/dag.go

@@ -526,7 +526,12 @@ func (woc *wfOperationCtx) resolveDependencyReferences(dagCtx *dagContext, task
 return nil, errors.InternalWrapError(err)
 }
 fstTmpl := fasttemplate.New(string(taskBytes), "{{", "}}")
- newTaskStr, err := common.Replace(fstTmpl, scope.replaceMap(), true)
+
+ replaceMap := scope.replaceMap()


We could change the name to something like prepareScope, here is an example where I do something similar:

https://github.com/argoproj/argo/blob/7cb2fd17765aad691eda25ca4c5acecb89f84394/workflow/controller/steps.go#L466-L471

This is minor though, I'll leave it up to you.

…resolution-with-with-param

dtaniwaki · 2020-04-22T13:16:36Z

@simster7 Could you review it again?

simster7 · 2020-04-22T17:34:09Z

util/params/params.go

@@ -0,0 +1,21 @@
+package params


This is a Workflow specific util, so it shouldn't be at the top-level util folder. Can you add this to the workflow/common package? You can add a new file there, but it should still be under the common package.

simster7 · 2020-04-22T17:34:44Z

util/params/params.go

@@ -0,0 +1,21 @@
+package params
+
+type Params map[string]string


Suggested change

type Params map[string]string

type Parameters map[string]string

Full name is more clear.

simster7 · 2020-04-22T17:36:39Z

workflow/common/util.go

@@ -300,19 +301,13 @@ func ProcessArgs(tmpl *wfv1.Template, args wfv1.ArgumentsProvider, globalParams,
 }

 // SubstituteParams returns a new copy of the template with global, pod, and input parameters substituted
-func SubstituteParams(tmpl *wfv1.Template, globalParams, localParams map[string]string) (*wfv1.Template, error) {
+func SubstituteParams(tmpl *wfv1.Template, globalParams, localParams params.Params) (*wfv1.Template, error) {


When we move the util file under common, we won't need the import prefix anymore.

simster7 · 2020-04-22T17:36:57Z

util/params/params_test.go

+ params := Params{"foo": "1"}
+ newParams := params.Clone()
+ assert.Equal(t, params, newParams)
+ assert.NotSame(t, &params, &newParams)


Nice, this is important.

simster7 · 2020-04-22T17:37:16Z

util/params/params.go

+ return newParams
+}
+
+func (ps Params) Clone() Params {


DeepCopy is the name we use for this in Argo.

simster7 · 2020-04-22T17:37:55Z

workflow/controller/scope.go

@@ -14,7 +15,7 @@ type wfScope struct {
 }

 // replaceMap returns a replacement map of strings intended to be used simple string substitution
-func (s *wfScope) replaceMap() map[string]string {
+func (s *wfScope) replaceMap() params.Params {
 replaceMap := make(map[string]string)


Shouldn't this be?

Suggested change

replaceMap := make(map[string]string)

replaceMap := make(params.Params)

simster7

Some more minor comments

simster7 · 2020-04-23T15:22:36Z

workflow/controller/scope.go

@@ -14,8 +15,8 @@ type wfScope struct {
 }

 // replaceMap returns a replacement map of strings intended to be used simple string substitution
-func (s *wfScope) replaceMap() map[string]string {
- replaceMap := make(map[string]string)
+func (s *wfScope) replaceMap() common.Parameters {


Please rename this function parameters() or getParameters() or something similar

simster7 · 2020-04-23T15:23:31Z

workflow/controller/workflowpod.go

- podParams[k] = v
- }
+func substitutePodParams(pod *apiv1.Pod, globalParams common.Parameters, tmpl *wfv1.Template) (*apiv1.Pod, error) {
+ podParams := globalParams.Merge()


Shouldn't this be DeepCopy if Merge has not arguments?

Suggested change

podParams := globalParams.Merge()

podParams := globalParams.DeepCopy()

dtaniwaki · 2020-04-23T16:21:20Z

@simster7 Fixed. Please check it again!

sonarcloud · 2020-04-23T16:24:22Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

95.2% Coverage
0.0% Duplication

simster7 · 2020-04-27T21:52:00Z

Back-ported to 2.7

Allow to use global params with withParam

cefcaa9

alexec requested a review from simster7 April 20, 2020 15:32

alexec assigned sarabala1979 Apr 20, 2020

simster7 reviewed Apr 20, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix-global-param-…

2c52b3e

…resolution-with-with-param

simster7 approved these changes Apr 21, 2020

View reviewed changes

dtaniwaki added 3 commits April 22, 2020 09:44

Merge remote-tracking branch 'upstream/master' into fix-global-param-…

1086c48

…resolution-with-with-param

Update testify to v1.5.1

b8bd1b7

Use params map as a custom type

398a861

simster7 suggested changes Apr 22, 2020

View reviewed changes

simster7 assigned simster7 and unassigned sarabala1979 Apr 22, 2020

dtaniwaki added 5 commits April 23, 2020 10:33

Rename copy method

2159866

Add comments for params

cf55127

Rename Params to Parameters

f5fd3ea

Initialize map as Parameters

435c5c6

Move Parameters to workflow common package

79a2e7d

simster7 reviewed Apr 23, 2020

View reviewed changes

dtaniwaki added 2 commits April 24, 2020 01:17

Rename replaceMap method

0f856e6

Use appropriate method for new empty parameters

d2cd75e

simster7 approved these changes Apr 23, 2020

View reviewed changes

simster7 merged commit e0f2697 into argoproj:master Apr 23, 2020

simster7 pushed a commit that referenced this pull request Apr 27, 2020

fix(controller): Include global params when using withParam (#2757)

b6022a9

dtaniwaki deleted the fix-global-param-resolution-with-with-param branch April 28, 2020 01:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(controller): allow to use global params with withParam #2757

fix(controller): allow to use global params with withParam #2757

dtaniwaki commented Apr 20, 2020 •

edited

Loading

simster7 Apr 20, 2020

simster7 Apr 20, 2020

dtaniwaki Apr 21, 2020

dtaniwaki Apr 21, 2020

simster7 Apr 21, 2020

dtaniwaki Apr 22, 2020

simster7 Apr 21, 2020

dtaniwaki commented Apr 22, 2020

simster7 Apr 22, 2020

simster7 Apr 22, 2020

simster7 Apr 22, 2020

simster7 Apr 22, 2020

simster7 Apr 22, 2020

simster7 Apr 22, 2020

simster7 left a comment

simster7 Apr 23, 2020

simster7 Apr 23, 2020

dtaniwaki commented Apr 23, 2020

sonarcloud bot commented Apr 23, 2020

simster7 commented Apr 27, 2020

	replaceMap := scope.replaceMap()
	replaceMap := scope.replaceMap(woc.globalParams)

	type Params map[string]string
	type Parameters map[string]string

	replaceMap := make(map[string]string)
	replaceMap := make(params.Params)

	podParams := globalParams.Merge()
	podParams := globalParams.DeepCopy()

fix(controller): allow to use global params with withParam #2757

fix(controller): allow to use global params with withParam #2757

Conversation

dtaniwaki commented Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtaniwaki commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simster7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtaniwaki commented Apr 23, 2020

sonarcloud bot commented Apr 23, 2020

simster7 commented Apr 27, 2020

dtaniwaki commented Apr 20, 2020 •

edited

Loading