parse arg commands at the top of dockerfiles #404
Conversation
pkg/dockerfile/buildargs.go
Outdated
| func (b *BuildArgs) AddMetaArgs(metaArgs map[string]string) *BuildArgs { | ||
| for key, value := range metaArgs { | ||
| logrus.Infof("META ARG: adding %s = %s", key, value) | ||
| b.AddMetaArg(key, &value) |
There was a problem hiding this comment.
I think the bug is here -- since there is a single value variable, the address it's pointing to is the same each time, so when that's passed in to AddMetaArg, all of the meta args end up pointing to the same value.
I think if you change it to
for key, value := range metaArgs {
v := value
b.AddMetaArg(key, &v)
}this should work?
There was a problem hiding this comment.
That was it. Thank you!
|
This should be ready for review. |
| @@ -14,7 +14,7 @@ | |||
|
|
|||
| # Bump these on release | |||
| VERSION_MAJOR ?= 0 | |||
| VERSION_MINOR ?= 3 | |||
| VERSION_MINOR ?= 5 | |||
There was a problem hiding this comment.
Thanks for catching this!! Must have missed it...many times... 😅
pkg/executor/build.go
Outdated
| @@ -250,12 +253,21 @@ func (s *stageBuilder) saveSnapshot(createdBy string, ck string, contents []byte | |||
| // DoBuild executes building the Dockerfile | |||
| func DoBuild(opts *config.KanikoOptions) (v1.Image, error) { | |||
| // Parse dockerfile and unpack base image to root | |||
| stages, err := dockerfile.Stages(opts) | |||
| stages, metaArgs, err := dockerfile.Stages(opts) | |||
There was a problem hiding this comment.
WDYT of storing the metaArgs for each stage in the KanikoStage type?
There was a problem hiding this comment.
yeah, that simplified things considerably. done.
pkg/snapshot/snapshot.go
Outdated
| filesAdded = true | ||
| if err := t.AddFileToTar(path); err != nil { | ||
| return false, err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if filesAdded && len(addedPaths) == 1 && addedPaths[0] == "/" { | ||
| logrus.Infof("Only added /, not taking snapshot.") |
There was a problem hiding this comment.
Why is this change needed?
There was a problem hiding this comment.
so for a command like RUN touch /${FOO}, where $FOO doesn't resolve to anything, there should just be an empty layer appended to the image, that's what docker does. For some reason, we see that / has changed and add that to the snapshot, resulting in an extra non-empty layer compared to docker.
There was a problem hiding this comment.
Hrmm, what if it's like "/foo/${FOO}"? Maybe the check is if the only added path is a directory, rather than root?
Fixes #395