Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Nov 15, 2024

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

return imageIDs, err
}

if b, ok := os.LookupEnv("COMPOSE_BAKE"); ok {
Copy link
Member

@thaJeztah thaJeztah Nov 15, 2024

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)

Copy link
Contributor Author

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

Copy link
Member

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".

@ndeloof ndeloof requested review from a team and glours and removed request for a team November 15, 2024 16:57
@ndeloof ndeloof force-pushed the delegate_build branch 3 times, most recently from 84fad3d to 9be4fef Compare November 18, 2024 08:52
@ndeloof ndeloof force-pushed the delegate_build branch 4 times, most recently from 164d6b2 to c678ddf Compare November 19, 2024 15:35
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 10.60606% with 177 lines in your changes missing coverage. Please review.

Project coverage is 49.66%. Comparing base (eba3ff8) to head (731da92).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
pkg/compose/build_bake.go 6.14% 166 Missing and 2 partials ⚠️
pkg/compose/build.go 52.63% 6 Missing and 3 partials ⚠️
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.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Comment on lines +50 to +52
if dockerCli.ConfigFile().Plugins["compose"]["build"] == "bake" {
b, ok = "true", true
}
Copy link
Contributor

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?

Copy link
Contributor

@glours glours left a 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

@ndeloof
Copy link
Contributor Author

ndeloof commented Nov 22, 2024

I was indeed looking for configuration on CI to run build-related tests with this new option enabled.

@thaJeztah
Copy link
Member

Just some quick blurbs;

  • Happy to see this being worked on again (thank you!)
  • I asked @laurazzard and others to have a peek at the cli-config part (we've been pretty bad at documenting conventions to use, so I asked them to have a peek)
  • One thing I'm wondering (not for this stage!) is if there'd be cases where we could even take out compose "in the middle", i.e., if we (in the CLI) could alias docker compose build to docker buildx bake (with the right options). Haven't wrapped my head around that option, but mostly considering that spinning up multiple binaries isn't always ideal, so if there's more optimizations to be made.

@ndeloof
Copy link
Contributor Author

ndeloof commented Nov 25, 2024

One thing I'm wondering (not for this stage!) is if there'd be cases where we could even take out compose "in the middle", i.e., if we (in the CLI) could alias docker compose build to docker buildx bake (with the right options). Haven't wrapped my head around that option, but mostly considering that spinning up multiple binaries isn't always ideal, so if there's more optimizations to be made.

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 up --build or watch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants