Allow build args in target image config #491
Conversation
load.go
Outdated
| 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
load.go
Outdated
| *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.
load.go
Outdated
|
|
||
| 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.
There was a problem hiding this comment.
Also base image is now deprecated in favor of the new bases.
| @@ -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.
|
@pmengelbert any update here? |
I'm going to update the PR to just allow the args in the labels. |
Signed-off-by: Peter Engelbert <[email protected]>
Signed-off-by: Peter Engelbert <[email protected]>
Also fix implementation Signed-off-by: Peter Engelbert <[email protected]>
Earlier commits allowed substitution in various places in the Image Config. This commit reverts most of those changes and allows arg substitution only in the `Labels` field of the image config. Signed-off-by: Peter Engelbert <[email protected]>
pmengelbert
force-pushed
the
pmengelbert/args_in_image_config/2
branch
from
676cf38 to
b0ade70
Compare
|
@cpuguy83 @MorrisLaw Now the args are only substituted in the |
|
I still need to update the tests |
|
Unit test is failing. |
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: