-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add initial "multi-stage" support in bashbrew #5929
Add initial "multi-stage" support in bashbrew #5929
Conversation
@@ -90,6 +92,8 @@ func cmdBuild(c *cli.Context) error { | |||
} | |||
defer archive.Close() | |||
|
|||
// TODO use "meta.StageNames" to do "docker build --target" so we can tag intermediate stages too for cache (streaming "git archive" directly to "docker build" makes that a little hard to accomplish without re-streaming) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this catch is worth pointing out explicitly here too -- we don't ever have a "local" copy of the context, and instead stream git archive
directly to docker build
, so even if docker build --target=N
were possible to do for a numbered stage, we'd still have an optimization problem implementing it (and I'd love to at least tag named stages that way, but the git archive | docker build
optimization makes that a little harder to swallow).
The practical implication of this is that build cache for these untagged stages will still be considered "ripe" by our tooling and thus we will end up spending extra time rebuilding things after each cleaning of "ripe" images on our build servers.
For this reason, I think that for the official images, we should plan to still discourage multi-stage image use unless the gains are clear and/or the final artifacts are reproducible (and thus can avoid unnecessary image updates), and even then encourage use of explicitly named stages so that we can hopefully tag them in the future to help avoid this issue.
05a4747
to
fa01936
Compare
@@ -53,7 +53,7 @@ _arches() { | |||
_froms() { | |||
bashbrew cat --format ' | |||
{{- range .TagEntries -}} | |||
{{- $.DockerFrom . -}} | |||
{{- $.DockerFroms . | join "\n" -}} | |||
{{- "\n" -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For folks affected by the DockerFrom
deprecation, in my experience porting my own scripts, this is the most common fix. (There was one place in the docs we needed to switch to .ArchLastStageFrom
instead, but it's pretty rare.)
@@ -115,7 +115,7 @@ template=' | |||
{{- "\n" -}} | |||
{{- range $.Entries -}} | |||
{{- $arch := .HasArchitecture arch | ternary arch (.Architectures | first) -}} | |||
{{- $from := $.ArchDockerFrom $arch . -}} | |||
{{- $froms := $.ArchDockerFroms $arch . -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one has me wondering if we should add an explicit .EnsureGitFetched
function, since we use this function in a lot of places to mean that. 😅
(Room for future improvement -- I don't think we should block this on that.)
I approve except we first need to have a document that will provide the very limited set on which we would accept a multi-stage build. The general idea is that if it can be reasonably done without multi-stage, it should be (long |
Initial rough list of allowable images (to help minimize problems due to #5929 (comment) / cache):
Maybe:
Pitfalls:
|
d49cca5
to
8f97fa1
Compare
8f97fa1
to
4d88a14
Compare
This allows bashbrew to properly handle cross-repository and cross-tag dependencies even in the face of multiple `FROM` instructions or `COPY --from=`. This also provides the scaffolding necessary to implement this in scripts using `bashbrew cat`. As fallback behavior, the `*DockerFrom` functions should return the `FROM` of the last stage in the `Dockerfile` (which is essentially the `FROM` of the final image). Also, the output of `bashbrew from` is now a space-separated list.
…e that's what's really necessary externally from that internal structure)
… of the given image (needed for docs generation)
… "bashbrew from" handling (especially in the case of no "--apply-constraints" flag)
…now that the command works properly)
…for a very dramatic speed increase (especially during dependency calculation)
98be20b
to
22c68d5
Compare
See docker-library/official-images#5929 (this is the initial supporting documentation for that PR).
Initial documentation PR up at docker-library/faq#6. 👍 🎉 🍰 |
This allows bashbrew to properly handle cross-repository and cross-tag dependencies even in the face of multiple
FROM
instructions orCOPY --from=
.This also provides the scaffolding necessary to implement this in scripts using
bashbrew cat
.As fallback behavior, the
*DockerFrom
functions should return theFROM
of the last stage in theDockerfile
(which is essentially theFROM
of the final image).Also, the output of
bashbrew from
is now a space-separated list.This is the first step towards #3383 -- there's still a lot more shell script work to be done (which is frankly harder to find since we don't get compiler errors when they aren't updated), but this is the first step that makes that work possible to do. 👍