ARG Command #162
ARG Command #162
Conversation
Can you explain why you had to modify that code? It will be difficult to carry a modification to it long term, and the diff is a bit hard to read. |
|
None of the buildargs methods are exported, so I pretty much just copied over the parts I needed. Not great but I couldn't think of a better way to do it... |
Could you try sending a PR to that repo to get them exported publicly? If that doesn't work, we'll need to checkin a patch file here to automate the change. |
|
For sure, it's exported now so I switched to vendoring it in. The changes are still kind of hard to read, but basically I changed It should be RFAL now |
cmd/executor/cmd/root.go
Outdated
| logrus.Error(err) | ||
| os.Exit(1) | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| type buildArg []string |
There was a problem hiding this comment.
nit: could you move this to another file in the same package?
| @@ -33,15 +35,16 @@ type CopyCommand struct { | |||
| snapshotFiles []string | |||
| } | |||
|
|
|||
| func (c *CopyCommand) ExecuteCommand(config *v1.Config) error { | |||
| func (c *CopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error { | |||
There was a problem hiding this comment.
nit: it's generally a best practice to not expose types like dockerfile.BuildArgs that come from a package dependency. Could we wrap this type with one of our own, and pass that around everywhere?
priyawadhwa
added
the
area/dockerfile-command
Begins to fix #14
To implement ARG, I modified the code here. I also created a BuildArgs type and passed it in to every command along with the config. Then, in each command, we get a list of ARG variables that aren't already defined in config.Env, and add them as values that can be used for environment replacement.
I didn't add functionality for specifying ARGs before FROM and replacing environment variables in FROM since this PR was getting big, so I was thinking I'd add that after this one goes in.