-
Notifications
You must be signed in to change notification settings - Fork 2.1k
build: enable platform flag for build if buildkit #1740
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1740 +/- ##
==========================================
- Coverage 56.04% 56.04% -0.01%
==========================================
Files 306 306
Lines 21050 21056 +6
==========================================
+ Hits 11798 11801 +3
- Misses 8399 8400 +1
- Partials 853 855 +2 |
Codecov Report
@@ Coverage Diff @@
## master #1740 +/- ##
==========================================
- Coverage 56.27% 56.04% -0.23%
==========================================
Files 307 306 -1
Lines 21131 21056 -75
==========================================
- Hits 11891 11801 -90
- Misses 8377 8400 +23
+ Partials 863 855 -8 |
| command.AddTrustVerificationFlags(flags, &options.untrusted, dockerCli.ContentTrustEnabled()) | ||
| command.AddPlatformFlag(flags, &options.platform) | ||
|
|
||
| flags.StringVar(&options.platform, "platform", os.Getenv("DOCKER_DEFAULT_PLATFORM"), "Set platform if server is multi-platform capable") |
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.
Can you document this environment-variable? https://github.com/docker/cli/blob/master/docs/reference/commandline/cli.md#environment-variables
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.
Added. Note that this PR does not add this env, but just moves around the code.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
70713eb to
2caffb1
Compare
thaJeztah
left a comment
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
|
|
||
| flags.StringVar(&options.platform, "platform", os.Getenv("DOCKER_DEFAULT_PLATFORM"), "Set platform if server is multi-platform capable") | ||
| // Platform is not experimental when BuildKit is used | ||
| buildkitEnabled, err := command.BuildKitEnabled(dockerCli.ServerInfo()) |
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.
I think this is buggy:
When we are creating commands and flags, we have not yet run PersistentPreRun, and so I think the CLI is not yet correctly initialized. I think docker --context=some-context build --platform ... can then have a very wierd behavior (where flag support is evaluated for a daemon that is not the one used after flags are actually parsed)
WDYT @thaJeztah @vdemeester @silvin-lubecki ?
Platform flag is supported if BuildKit is being used. If not (lcow) then I left it for the old behavior. I don't think we want to move this flag out of experimental for other commands like pull and run cause their implementation will change with future containerd storage integration.
@tiborvass
Signed-off-by: Tonis Tiigi tonistiigi@gmail.com