Hide command options that are related to Windows by boaz0 · Pull Request #30788 · moby/moby

boaz0 · 2017-02-07T13:41:12Z

- What I did

On Linux hide commands options that are relevant to Windows.

- How I did it

Add the Server-OS header to the ping response which will be used to determine what options to hide
Set the os annotation containing the operating system name
Hide all options that has this annotation and the value is not equal to the operating system name

- How to verify it
🤔

boaz0 · 2017-02-07T13:42:53Z

//cc @albers @thaJeztah @dnephin
closes #30624

dnephin · 2017-02-07T15:55:20Z

cli/command/image/build.go

--squash is not windows only, I think it was just being used as an example of an experimental flag.

yes, that's true.

dnephin · 2017-02-07T15:55:29Z

cli/command/stack/opts.go

same here, not windows only

rather linux only at the moment 😅

dnephin · 2017-02-07T15:57:59Z

client/ping.go

This seems kind of hacky, I can see this failing unexpectedly.

I think it would be better to add a new header with the serverOS in a format that is easier to parse (or doesn't need to be parsed at all).

dnephin · 2017-02-07T16:00:21Z

cmd/docker/docker.go

We use this Option naming convention for data objects populated from command line flags. This struct is not being used that way, so I think the name is confusing.

How about apiProperties or apiFeatures or something like that?

aaronlehmann · 2017-02-07T21:25:06Z

cmd/docker/docker.go

There's an extra blank line above "strings".

Yes, I know. vim-go does that automatically. I can fix that, though.

EDIT to avoid overloading the Jenkins master I will push this change only if there will be at least one more comment about the design.
Thanks.

thaJeztah · 2017-02-22T11:53:44Z

Looking at the output of the /info endpoint;

  "KernelVersion": "4.9.10-moby",
  "OperatingSystem": "Alpine Linux v3.5 (containerized)",
  "OSType": "linux",
  "Architecture": "x86_64",

I wonder if we should use the same terminology (OSType / OS-Type) for this

boaz0 · 2017-02-22T12:01:20Z

@thaJeztah sounds right. I know OSType is also used when displaying containers statistics.
I will change that.

thaJeztah

design LGTM, left some comments inline

thaJeztah · 2017-02-28T10:54:33Z

cmd/docker/docker.go

Can you include this in the isFlagSupported() function? e.g.

if !isFlagSupported(f, apiProps.OSType, apiProps.ClientVer) { f.Hidden = true }

This logic should take older daemons into account; older daemons won't return an OS-Type header. As a result, this logic will hide any flag that has a os annotation set. If the daemon didn't return an OS-Type header, just show the flag.

Sure, I am surprised I missed it. Thanks for noticing me!

thaJeztah · 2017-02-28T11:05:08Z

cli/command/container/opts.go

nit: Should we name this "ostype" / "os-type" as well to be consistent (otherwise there's a risk they're getting different meanings)?

thaJeztah · 2017-03-01T14:49:36Z

cmd/docker/docker.go

nit; using an or here is probably clearer;

if !isOSTypeSupported(f, osType) || !isVersionSupported(f, clientVersion) { f.Hidden = true }

thaJeztah

LGTM, thank you!

As a follow-up we may want to look into hiding Linux flags on Windows. Especially on docker run there's a lot of flags that are Linux-specific, and I think it'll be a nice enhancements to hide those when running against a Windows daemon 👍

boaz0 · 2017-03-01T14:59:17Z

@thaJeztah sure thing!

thaJeztah · 2017-03-10T21:33:55Z

ping @dnephin ptal

@ripcurld0 heard you're a git rebase pro 😉

boaz0 · 2017-03-11T17:36:55Z

indeed, i am 🥇

Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

dnephin

LGTM

dnephin · 2017-03-13T15:09:25Z

cli/command/cli.go

+// OSType returns the operating system the daemon is running on.
+func (cli *DockerCli) OSType() string {
+	return cli.osType
+}


I'm now realizing that we never should have put these things on DockerCli directly. We should have created a new struct to store experimental and default version. That would make the signature for isSupported() and the others a lot cleaner.

I'll handle that in a new PR.

vdemeester

LGTM 🦁

Hide command options that are related to Windows

GordonTheTurtle added the status/0-triage label Feb 7, 2017

vdemeester added status/1-design-review and removed status/0-triage labels Feb 7, 2017

dnephin reviewed Feb 7, 2017

View reviewed changes

aaronlehmann reviewed Feb 7, 2017

View reviewed changes

GordonTheTurtle assigned thaJeztah Feb 22, 2017

thaJeztah requested changes Feb 28, 2017

View reviewed changes

thaJeztah added status/2-code-review and removed status/1-design-review labels Feb 28, 2017

thaJeztah reviewed Mar 1, 2017

View reviewed changes

thaJeztah approved these changes Mar 1, 2017

View reviewed changes

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Mar 1, 2017

thaJeztah mentioned this pull request Mar 10, 2017

Windows: Balk on --privileged #31740

Merged

Hide command options that are related to Windows

7199522

Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

dnephin approved these changes Mar 13, 2017

View reviewed changes

vdemeester approved these changes Mar 13, 2017

View reviewed changes

vdemeester merged commit be453c5 into moby:master Mar 13, 2017

GordonTheTurtle added this to the 17.04.0 milestone Mar 13, 2017

thaJeztah added the impact/changelog label Mar 13, 2017

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30788 from ripcurld0/hide_win_opts

278e4cc

Hide command options that are related to Windows

thaJeztah mentioned this pull request Jul 15, 2017

[Feature request] Hide Windows specific options in --help if not applicable #30624

Closed

This was referenced Jun 27, 2025

Remove dependency on lazyregexp from client #50272

Closed

client: remove getDockerOS utility in favor of "Ostype" header #50276

Merged

Conversation

boaz0 commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boaz0 commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boaz0 Feb 7, 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!

thaJeztah commented Feb 22, 2017

Uh oh!

boaz0 commented Feb 22, 2017

Uh oh!

thaJeztah left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

boaz0 commented Mar 1, 2017

Uh oh!

thaJeztah commented Mar 10, 2017

Uh oh!

boaz0 commented Mar 11, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

boaz0 commented Feb 7, 2017 •

edited

Loading

boaz0 commented Feb 7, 2017 •

edited

Loading

boaz0 Feb 7, 2017 •

edited

Loading

thaJeztah left a comment •

edited

Loading