Allow build args in target image config #491
Conversation
Signed-off-by: Peter Engelbert <[email protected]>
Signed-off-by: Peter Engelbert <[email protected]>
Also fix implementation Signed-off-by: Peter Engelbert <[email protected]>
| img.Labels[k] = updated | ||
| } | ||
|
|
||
| if img.Post != nil { |
There was a problem hiding this comment.
I don't see why this would be necessary and would prefer to not support it.
There was a problem hiding this comment.
Understandable. Not strictly necessary, just a little more convenient for certain things. If you don't agree with the rationale feel free to close
| *p = updated | ||
| } | ||
|
|
||
| for i, s := range img.Env { |
There was a problem hiding this comment.
I'm a little hesitant support this here but not against it.
|
|
||
| func (img *ImageConfig) processBuildArgs(lex *shell.Lex, args map[string]string, allowArg func(string) bool) error { | ||
| var errs error | ||
| for _, p := range []*string{&img.Base, &img.Cmd, &img.User, &img.Entrypoint, &img.StopSignal, &img.WorkingDir} { |
There was a problem hiding this comment.
Likewise, I don't see why image base would need build args.
There was a problem hiding this comment.
At the very least I think it would be useful for the tag. Some repos do not have a latest tag, and not allowing a build arg here forces a hard-coded tag. I'm not married to the idea of introducing this, but it does seem useful.
Other uses could be not hard-coding a registry.
There was a problem hiding this comment.
Not hard-coding a registry could be taken care of by source policies.
There was a problem hiding this comment.
What do you think about tags?
There was a problem hiding this comment.
I think I'd opt for explicitness in the spec in this case until someone actually asks for this.
| @@ -447,3 +453,57 @@ func (b *ArtifactBuild) processBuildArgs(lex *shell.Lex, args map[string]string, | |||
|
|
|||
| return goerrors.Join(errs...) | |||
| } | |||
|
|
|||
| func (img *ImageConfig) processBuildArgs(lex *shell.Lex, args map[string]string, allowArg func(string) bool) error { | |||
There was a problem hiding this comment.
Labels seems like a good candidate for build args. But why the other fields? Why not just limit support to Labels for now until there's a compelling use case/need for additional build arg support?
Note: I could be missing context.
There was a problem hiding this comment.
See above, I think it could be useful for setting base image tags or registry host, particularly when the base image provider doesn't have a latest tag.
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #490
Fixes #487
Special notes for your reviewer: