Skip to content

add support of --builder and BUILDX_BUILDER#10745

Merged
glours merged 1 commit intodocker:v2from
glours:add-builder-support
Jul 3, 2023
Merged

add support of --builder and BUILDX_BUILDER#10745
glours merged 1 commit intodocker:v2from
glours:add-builder-support

Conversation

@glours
Copy link
Copy Markdown
Contributor

@glours glours commented Jun 27, 2023

What I did
Add --builder flag to build command and support of the BUILDX_BUILDER env variable to support choose dedicated builder.

Should address #10664 until a proposal is discussed in the Compose specification

Related issue
N/A

(not mandatory) A picture of a cute animal, if possible in relation to what you did
image

@glours glours self-assigned this Jun 27, 2023
silvin-lubecki
silvin-lubecki approved these changes Jun 27, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 😻

thaJeztah
thaJeztah reviewed Jun 27, 2023
View reviewed changes
cmd/compose/build.go
cmd.Flags().StringVar(&opts.ssh, "ssh", "", "Set SSH authentications used when building service images. (use 'default' for using your default SSH Agent)")
cmd.Flags().StringVar(&opts.builder, "builder", "", "Set builder to use.")
cmd.Flags().Bool("parallel", true, "Build images in parallel. DEPRECATED")
cmd.Flags().MarkHidden("parallel") //nolint:errcheck
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very unrelated, but if this flag is deprecated, should this use MarkDeprecated()? (are we still using the value anywhere?)

Copy link
Copy Markdown
Contributor Author

@glours glours Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do a dedicated PR for this one 👍

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Definitely not in this PR (my eye just dropped on it .. well.. as always 😂)

@glours glours force-pushed the add-builder-support branch 2 times, most recently from 12cf2f4 to bcc1974 Compare June 27, 2023 12:58
ndeloof
ndeloof approved these changes Jun 27, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but shall we also introduce a builder attribute in the compose specification?

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 27, 2023

shall we also introduce a builder attribute in the compose specification?

Would that be very useful? If we'd add such an option, I think it'd make more sense to describe the properties of the builder instead of the name of the builder. Currently it's somewhat similar to --host, --context, etc, which tend to be more "runtime" / "invocation" options than something you want to "bake" into your project's (compose file) config.

Perhaps we need to resurrect some of the concepts from docker app, where such (environment-specific) parameters could be kept in a separate file.

@ndeloof
Copy link
Copy Markdown
Contributor

ndeloof commented Jun 27, 2023

It would indeed be nice there's some way to describe a builder so that we can inspect the available ones and select a good match, but afaik we only can rely on a name. Also, I can imagine it will become quickly boring for a user to pass --builder foo every time he wants to build image, also would need this to be available on compose up... while this could just be set once in the compose.yaml file.
Last but not least, an image might require a specific builder to be used, but not the others

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 27, 2023

That's where the env var would be for? (export BUILDX_BUILDER=....)

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 27, 2023

Last but not least, an image might require a specific builder to be used, but not the others

Yes, that's something to look into I guess. I'm still not sure if hard-coding it in the project's config would make sense (as it's effectively hard-coding your current environment into the project, which seems better to handle separate).

milas
milas approved these changes Jun 27, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, it'd be nice to have the flag available on up as well, but logic LGTM!

@glours glours force-pushed the add-builder-support branch from bcc1974 to bdd3757 Compare July 3, 2023 07:30
@glours
Copy link
Copy Markdown
Contributor Author

glours commented Jul 3, 2023

For consistency, it'd be nice to have the flag available on up as well, but logic LGTM!

I'm not sure we should add this flag to the up, the consistency should be to keep all the build flags to the build command only, IHMO, WDYT?

@glours glours force-pushed the add-builder-support branch from bdd3757 to f45c5cb Compare July 3, 2023 07:57
@ndeloof
Copy link
Copy Markdown
Contributor

ndeloof commented Jul 3, 2023

Agree advanced build options should be limited to build command. This is also why I suggest we get such attributes introduce in compose.yaml

@glours
add support of --builder and BUILDX_BUILDER
28301fb 
Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 3, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (fa3e16c) 59.09% compared to head (f45c5cb) 59.10%.

❗ Current head f45c5cb differs from pull request most recent head 28301fb. Consider uploading reports for the commit 28301fb to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##               v2   #10745      +/-   ##
==========================================
+ Coverage   59.09%   59.10%   +0.01%     
==========================================
  Files         115      115              
  Lines        9844     9850       +6     
==========================================
+ Hits         5817     5822       +5     
  Misses       3436     3436              
- Partials      591      592       +1
Impacted Files Coverage Δ
pkg/api/api.go 43.39% <ø> (ø)
cmd/compose/build.go 83.33% <100.00%> (+1.38%) ⬆️
pkg/compose/build.go 72.77% <100.00%> (ø)
pkg/compose/build_buildkit.go 34.61% <100.00%> (+5.76%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@glours glours force-pushed the add-builder-support branch from f45c5cb to 28301fb Compare July 3, 2023 08:11
@glours glours enabled auto-merge July 3, 2023 08:13
@glours glours merged commit 827e864 into docker:v2 Jul 3, 2023
@karolz-ms karolz-ms mentioned this pull request Jul 12, 2023
Closed
@TBBle TBBle mentioned this pull request Jan 3, 2024
Open
@glours glours deleted the add-builder-support branch June 30, 2025 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah left review comments

@ndeloof ndeloof ndeloof approved these changes

+2 more reviewers

@milas milas milas approved these changes

@silvin-lubecki silvin-lubecki silvin-lubecki approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@glours glours

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@glours @thaJeztah @ndeloof @milas @silvin-lubecki