-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
delegate build to buildx bake #12300
base: main
Are you sure you want to change the base?
Conversation
pkg/compose/build.go
Outdated
return imageIDs, err | ||
} | ||
|
||
if b, ok := os.LookupEnv("COMPOSE_BAKE"); ok { |
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.
Perhaps could also be worth (if compose reads the cli config) to consider either an option in features
, or plugins
(plugins allows plugin-specific options to be set), which would allow opt-in/opt-out of this without having to use an env-var;
https://github.com/docker/cli/blob/9861ce90fd6b8ddca19db5f803dcbef9a583e9e1/cli/config/configfile/file.go#L42-L44
Plugins map[string]map[string]string `json:"plugins,omitempty"`
Aliases map[string]string `json:"aliases,omitempty"`
Features map[string]string `json:"features,omitempty"`
(in addition to an env-var probably)
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.
we never used this mechanism in the past for optional/experimental docker compose features
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.
Yeah, my thinking here was that the cli-config would more easily allow this to be set as a default, which could also allow (e.g.) it to be set through docker desktop "settings".
84fad3d
to
9be4fef
Compare
164d6b2
to
c678ddf
Compare
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
c678ddf
to
731da92
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12300 +/- ##
==========================================
- Coverage 50.14% 49.66% -0.48%
==========================================
Files 154 155 +1
Lines 15060 15310 +250
==========================================
+ Hits 7552 7604 +52
- Misses 6738 6931 +193
- Partials 770 775 +5 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
if dockerCli.ConfigFile().Plugins["compose"]["build"] == "bake" { | ||
b, ok = "true", true | ||
} |
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.
SGTM, but maybe we should error out on unexpected/invalid values?
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.
LGTM, tests ok
We should open follow up PRs to add bake flavour of existing e2e build tests
I was indeed looking for configuration on CI to run build-related tests with this new option enabled. |
Just some quick blurbs;
|
I don't think this is a good solution. 1st, this would break separation of concerns, and force the docker CLI to be aware about compose sub-commands and UX (only a subset of service might be requested for build). Also, this would interfere with other commands which involve building images but also some compose actions, typically |
What I did
introduce support for COMPOSE_BAKE to opt-in for build to be delegated to buildx bake. Compose generates a bake json config from application model and executes bake, which (we assume) better knows how to orchestrate build steps for best efficiency.
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did