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

add support of --builder and BUILDX_BUILDER #10745

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

glours
Copy link
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
Copy link
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 😻

@@ -101,6 +103,7 @@ func buildCommand(p *ProjectOptions, progress *string, backend api.Service) *cob
cmd.Flags().BoolVar(&opts.pull, "pull", false, "Always attempt to pull a newer version of the image.")
cmd.Flags().StringArrayVar(&opts.args, "build-arg", []string{}, "Set build-time variables for services.")
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
Member

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
Contributor Author

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
Member

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
Copy link
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
Member

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
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
Member

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

@thaJeztah
Copy link
Member

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

Copy link
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
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
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

@codecov
Copy link

codecov bot commented Jul 3, 2023

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.

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.

5 participants