Fix COPY/ADD --from= with ARG in stage scope#6730
Fix COPY/ADD --from= with ARG in stage scope#6730Honny1 wants to merge 1 commit intocontainers:mainfrom
COPY/ADD --from= with ARG in stage scope#6730Conversation
Fixes: containers#6719 Signed-off-by: Jan Rodák <hony.com@seznam.cz>
dosubot
bot
added
the
size:L
nalind
left a comment
nalind
left a comment
There was a problem hiding this comment.
Generally looks good, but questions about handling precedence when an ARG is provided a default value both in the header and in a stage and how the ARG value is de-quoted.
| // not record the right value here. | ||
| rootfs := after | ||
| b.rootfsMap[rootfs] = struct{}{} | ||
| logrus.Debugf("rootfs needed for COPY in stage %d: %q", stageIndex, rootfs) |
There was a problem hiding this comment.
Are these lines and the preceding comment still correct and/or necessary given the addition being made below?
| // specified but default value is set in Containerfile | ||
| // via `ARG key=value` so default value can be used. | ||
| userArgs = slices.Concat(builtinArgs, userArgs, headingArgs) | ||
| userArgs = slices.Concat(builtinArgs, userArgs, headingArgs, localScopeArgs) |
There was a problem hiding this comment.
How does this handle conflicting values, when one should override another? For example, what happens when tests/bud/copy-from-stage-scoped-arg/Containerfile.multi-copy-arg-override has ARG VARIANT=dev added to its header, before the "build" stage?
| if arg != nil { | ||
| argName, argValue, hasValue := strings.Cut(arg.Value, "=") | ||
| if hasValue && argName != "" { | ||
| stageLocalScopeArgs[argName] = strings.Trim(argValue, "\"'") |
There was a problem hiding this comment.
Probably want to use imagebuilder.ProcessWord() to handle the quoting here.
There was a problem hiding this comment.
I was just reviewing another PR, and was reminded by openshift/imagebuilder#210 that there might be multiple ARGs defined on this line.
2 participants
Fixes: #6719
What type of PR is this?
/kind bug
What this PR does / why we need it:
How to verify it
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?