LCOW: CLI changes to add platform flag - pull, run, create and build by lowenna · Pull Request #474 · docker/cli

lowenna · 2017-08-27T01:56:56Z

Signed-off-by: John Howard jhoward@microsoft.com

Replacement for #416. This adds the CLI flag changes to match those described in moby/moby#34617, and PR'd to moby/moby in moby/moby#34642.

It adds --platform to pull, run, create, and build. Note import requires further daemon changes so has not been done yet (will be in a further PR once the daemon has been updated).

dnephin · 2017-08-28T17:17:48Z

 hide(container.NewLogsCommand(dockerCli)),
 hide(container.NewPauseCommand(dockerCli)),
 hide(container.NewPortCommand(dockerCli)),
+ hide(image.NewPullCommand(dockerCli)),


looks like this moved for no reason? now it's not sorted

dnephin · 2017-08-28T17:20:16Z

 	hostConfig := containerConfig.HostConfig
 	stdout, stderr := dockerCli.Out(), dockerCli.Err()
 	client := dockerCli.Client()
+	ociPlatform := system.ParsePlatform(opts.platform)


I think this should be parsed by the flag instead after flag parsing. To accomplish this you can create a new PlatformOpt type in opts/platform.go with a func Value() spec.Platform to return the parsed type.
The field in the options struct will be a PlatformOpt instead of a string

dnephin · 2017-08-28T17:21:47Z

 	flags.StringVar(&opts.detachKeys, "detach-keys", "", "Override the key sequence for detaching a container")

+	flags.StringVar(&opts.platform, "platform", "", "Set platform if server is multi-platform capable")
+	flags.SetAnnotation("platform", "version", []string{"1.32"})


Since this is going to be repeated in a bunch of commands how about we move this to
funcaddPlatformFlag(flags*pflag.FlagSet, opt*PlatformOpt)
in cli/flags/common.go
There will also need to be another annotation for hiding based on the header:
flags.SetAnnotation("platform", "platform", nil)
The code to hide flags based on annotations is in cmd/docker/docker.go

dnephin · 2017-08-28T17:22:13Z

 NewLoadCommand(dockerCli),
- NewPullCommand(dockerCli),
 NewPushCommand(dockerCli),
+ NewPullCommand(dockerCli),


dnephin · 2017-08-28T17:25:51Z

-func imagePullPrivileged(ctx context.Context, cli command.Cli, authConfig types.AuthConfig, ref string, requestPrivilege types.RequestPrivilegeFunc, all bool) error {
+func imagePullPrivileged(ctx context.Context, cli command.Cli, authConfig types.AuthConfig, ref string, requestPrivilege types.RequestPrivilegeFunc, all bool, platform specs.Platform) error {
+	if platform.OS == "" && os.Getenv("DOCKER_DEFAULT_PLATFORM") != "" {
+ platform.OS = os.Getenv("DOCKER_DEFAULT_PLATFORM")


This should be handled somewhere else before it gets into the pull handling. Possibly as part of the PlatformOpt type. It can be used as a default value for Value()
This will also remove the duplication.

lowenna · 2017-09-19T01:45:38Z

Rebased and reworked based on moby/moby#34642 and addressed most of the comments. Still a work in progress until 34642 is merged in moby/moby as the vendoring will remain hacked.

codecov-io · 2017-10-30T18:47:38Z

Codecov Report

Merging #474 into master will decrease coverage by 0.55%.
The diff coverage is 45.45%.

@@ Coverage Diff @@## master #474 +/- ##
==========================================
- Coverage 50.1% 49.54% -0.56% 
==========================================
Files 216 211 -5 Lines 17700 17480 -220 ==========================================
- Hits 8868 8661 -207 + Misses 8388 8386 -2 + Partials 444 433 -11

Impacted Files	Coverage Δ
cli/command/container/run.go	`0% <0%> (ø)`	⬆️
cli/command/utils.go	`0% <0%> (ø)`	⬆️
cli/command/image/build.go	`41.64% <100%> (+0.3%)`	⬆️
cli/command/container/create.go	`45.39% <50%> (+0.05%)`	⬆️
cli/command/image/trust.go	`21.78% <50%> (ø)`	⬆️
cli/command/image/pull.go	`77.08% <75%> (+0.99%)`	⬆️
cli/compose/convert/service.go	`35.41% <0%> (-1.69%)`	⬇️
cli/compose/loader/loader.go	`82.03% <0%> (-0.11%)`	⬇️
cli/command/trust/cmd.go	`0% <0%> (ø)`	⬆️
cli/trust/trust.go	`10% <0%> (ø)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dc89aa...d8b7825. Read the comment docs.

lowenna · 2017-10-30T18:48:25Z

Rebased to address conflicts, and reworked to use two utility functions rather than replicate the same code in a few places.

Ready for review, removed WIP from the title, updated the commit comment and the description of the PR. @vdemeester can this be moved to code review now?

lowenna · 2017-10-30T18:53:07Z

@johnstep PTAL too. Thanks.

dnephin

Design LGTM

I still think we should hide this flag when the list of supported platforms returned by the API has a length of 1.

That would be handled by adding another field to ServerInfo in cli/command/cli.go, and calling a new function from hideUnsupportedFeatures()incmd/docker/docker.go`.

dnephin · 2017-10-30T18:57:13Z

+
+// PlatformFromEnvironment sets the platform from an environment variable
+// if it isn't otherwise populated.
+func PlatformFromEnvironment(platform string) string {


Since this is effectively setting a default value for platform I think the environment variable should be read as the default value in AddPlatformFlag() instead of exporting this function:
flags.StringVar(target, "platform", os.Getenv("DOCKER_DEFAULT_PLATFORM"), ...)

That makes sense. Updated.
For hiding the flag, it is going to require getting consensus on moby/moby#34628 first, which I don't think there is among other maintainers yet.

dnephin

LGTM

johnstep · 2017-10-31T02:05:37Z

This should probably be hidden for a daemon without experimental.

Signed-off-by: John Howard <jhoward@microsoft.com> This is the CLI updates for the document discussed in moby/moby#34617 to support Linux Containers on Windows. It adds --platform= as CLI flags to the four commands listed above. Import still to be completed (needs daemon changes).

lowenna · 2017-10-31T16:51:44Z

@johnstep - OK, added that annotation in utils.go::AddPlatformFlag() and verified it operates as expected.

johnstep

LGTM

lowenna · 2017-11-03T17:08:02Z

Is this OK to merge with 2 LGTM's?

vdemeester

LGTM 🐮

thaJeztah

LGTM

GordonTheTurtle added the status/0-triage label Aug 27, 2017

lowenna mentioned this pull request Aug 27, 2017
LCOW: API: Add platform to /images/create and /build moby/moby#34642
Merged

dnephin reviewed Aug 28, 2017
View reviewed changes

lowenna force-pushed the jjh/apis-for-platform branch from 5f89aff to 03fb697Compare September 13, 2017 20:07

lowenna force-pushed the jjh/apis-for-platform branch from 03fb697 to d186bc3Compare September 19, 2017 01:46

vdemeester added status/1-design-review and removed status/0-triage labels Sep 20, 2017

lowenna force-pushed the jjh/apis-for-platform branch 5 times, most recently from 8f7375e to 8abdebaCompare October 30, 2017 18:44

lowenna changed the title ~~[WIP] - Pending moby/moby - LCOW: CLI changes to add platform flag.~~LCOW: CLI changes to add platform flag - pull, run, create and buildOct 30, 2017

dnephin reviewed Oct 30, 2017
View reviewed changes

lowenna force-pushed the jjh/apis-for-platform branch from 8abdeba to a5e4e1dCompare October 30, 2017 20:59

dnephin approved these changes Oct 30, 2017
View reviewed changes

lowenna force-pushed the jjh/apis-for-platform branch from a5e4e1d to d8b7825Compare October 31, 2017 16:50

dnephin added status/2-code-review and removed status/1-design-review labels Oct 31, 2017

johnstep approved these changes Nov 1, 2017
View reviewed changes

vdemeester approved these changes Nov 7, 2017
View reviewed changes

vdemeester requested a review from thaJeztah November 7, 2017 13:23

thaJeztah approved these changes Nov 7, 2017
View reviewed changes

thaJeztah added kind/experimental platform/windows labels Nov 7, 2017

thaJeztah merged commit 042575a into docker:masterNov 7, 2017

GordonTheTurtle added this to the 17.12.0 milestone Nov 7, 2017

lowenna deleted the jjh/apis-for-platform branch November 7, 2017 16:21

albers mentioned this pull request Nov 20, 2017
Add bash completion for --platform#704
Merged

carlfischer1 mentioned this pull request Jan 9, 2018
docker-compose build failed on LCOW mode docker/compose#5548
Closed

This was referenced Jan 9, 2018
The following environment variable is an interim imdone/moby#5
Open
The following environment variable is an interim imdone/moby#31
Open

Uh oh!

Conversation

lowenna commented Aug 27, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephinAug 28, 2017

Choose a reason for hiding this comment

Uh oh!

dnephinAug 28, 2017

Choose a reason for hiding this comment

Uh oh!

dnephinAug 28, 2017

Choose a reason for hiding this comment

Uh oh!

dnephinAug 28, 2017

Choose a reason for hiding this comment

Uh oh!

dnephinAug 28, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lowenna commented Sep 19, 2017

Uh oh!

codecov-io commented Oct 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lowenna commented Oct 30, 2017

Uh oh!

lowenna commented Oct 30, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

dnephinOct 30, 2017

Choose a reason for hiding this comment

Uh oh!

lowennaOct 30, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

johnstep commented Oct 31, 2017

Uh oh!

lowenna commented Oct 31, 2017

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

lowenna commented Nov 3, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lowenna commented Aug 27, 2017•
edited
Loading

dnephinAug 28, 2017•
edited
Loading

codecov-io commented Oct 30, 2017•
edited
Loading