Support multi stage builds#141
Conversation
dlorenc
left a comment
dlorenc
left a comment
There was a problem hiding this comment.
Makes sense overall, a few high level comments. I'll take a closer look once we rebase this on top of the go-containers switch.
| logrus.Infof("dest: %s", dest) | ||
|
|
||
| // Resolve from | ||
| if c.cmd.From != "" { |
There was a problem hiding this comment.
What if multiple steps have the same from line? Should we add any randomness here?
There was a problem hiding this comment.
Sorry, what do you mean by randomness?
Right now, if "from" is specified as a previous stage, we change the build context to be the path to the directory where files from that stage are saved. That directory should remain unchanged even if multiple copy commands refer back to it
There was a problem hiding this comment.
I was worried about a case like this:
FROM base1
RUN something
FROM base1
RUN somethingelse
FROM base2
COPY --from=0
But I guess we can't have conflicts here, because the users have to refer to them by index (or alias)
| nameToIndex := make(map[string]string) | ||
| for i, stage := range stages { | ||
| index := strconv.Itoa(i) | ||
| nameToIndex[stage.Name] = index |
There was a problem hiding this comment.
it looks like the second one here overwrites the first one. Do we need both?
There was a problem hiding this comment.
so if the stage has a name associated the second won't override the first, so I added both in just to make it easier to resolve '--from' from names to indexes later on
There was a problem hiding this comment.
Ah ok. Maybe it would be more clear to have two separate maps?
There was a problem hiding this comment.
I guess it would get confusing if someone tried to name a stage with an index...
There was a problem hiding this comment.
Oh I see, that makes sense -- I changed it a bit to be less confusing with one map
| if stageIndex <= index { | ||
| continue | ||
| } | ||
| ms, err := image.NewSourceImage(stage.BaseName) |
There was a problem hiding this comment.
This will have to get rewritten a bit after my go-containerregistry PR goes in.
|
This is RFAL! |
| srcContext string | ||
| snapshotMode string | ||
| bucket string | ||
| logLevel string |
There was a problem hiding this comment.
looks like you dropped the dockerinsecuretlsverify in a merge conflict.
There was a problem hiding this comment.
yah, it didn't look like it was doing anything now that we switched to go-containerregistry
There was a problem hiding this comment.
(probably should have done that in a different commit)
| logrus.Infof("dest: %s", dest) | ||
|
|
||
| // Resolve from | ||
| if c.cmd.From != "" { |
There was a problem hiding this comment.
I was worried about a case like this:
FROM base1
RUN something
FROM base1
RUN somethingelse
FROM base2
COPY --from=0
But I guess we can't have conflicts here, because the users have to refer to them by index (or alias)
| nameToIndex := make(map[string]string) | ||
| for i, stage := range stages { | ||
| index := strconv.Itoa(i) | ||
| nameToIndex[stage.Name] = index |
There was a problem hiding this comment.
Ah ok. Maybe it would be more clear to have two separate maps?
| nameToIndex := make(map[string]string) | ||
| for i, stage := range stages { | ||
| index := strconv.Itoa(i) | ||
| nameToIndex[stage.Name] = index |
There was a problem hiding this comment.
I guess it would get confusing if someone tried to name a stage with an index...
| switch c := cmd.(type) { | ||
| case *instructions.EnvCommand: | ||
| envCommand := commands.NewEnvCommand(c) | ||
| if err := envCommand.ExecuteCommand(&imageConfig.Config); err != nil { |
There was a problem hiding this comment.
Why are we executing ENV commands in here? It seems like this shouldn't have any side effects.
There was a problem hiding this comment.
so we need to keep track of which ENVs were declared before a copy command for a case like this:
FROM base1
RUN something
FROM base2
ENV file file
COPY --from=0 $file ./
So here I'm executing any EnvCommands on a temporary config for that stage, and that updated config is used to evaluate dependencies in any subsequent COPY commands
|
PTAL |
priyawadhwa
added
the
area/multi-stage builds
4 participants
Fixes #4 and partially #9