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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide command options that are related to Windows #30788

Merged
merged 1 commit into from Mar 13, 2017

Conversation

@boaz1337
Contributor

boaz1337 commented Feb 7, 2017

- 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
馃

@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337
Contributor

boaz1337 commented Feb 7, 2017

Show outdated Hide outdated cli/command/image/build.go Outdated
Show outdated Hide outdated cli/command/stack/opts.go Outdated
Show outdated Hide outdated client/ping.go Outdated
Show outdated Hide outdated cmd/docker/docker.go Outdated
Show outdated Hide outdated cmd/docker/docker.go Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 22, 2017

Member

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

Member

thaJeztah commented Feb 22, 2017

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

@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 Feb 22, 2017

Contributor

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

Contributor

boaz1337 commented Feb 22, 2017

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

@thaJeztah

thaJeztah requested changes Feb 28, 2017 edited

design LGTM, left some comments inline

Show outdated Hide outdated cmd/docker/docker.go Outdated
Show outdated Hide outdated cli/command/container/opts.go Outdated
Show outdated Hide outdated cmd/docker/docker.go Outdated
@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 馃憤

@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 Mar 1, 2017

Contributor

@thaJeztah sure thing!

Contributor

boaz1337 commented Mar 1, 2017

@thaJeztah sure thing!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 10, 2017

Member

ping @dnephin ptal

@ripcurld0 heard you're a git rebase pro 馃槈

Member

thaJeztah commented Mar 10, 2017

ping @dnephin ptal

@ripcurld0 heard you're a git rebase pro 馃槈

@boaz1337

This comment has been minimized.

Show comment
Hide comment
@boaz1337

boaz1337 Mar 11, 2017

Contributor

indeed, i am 馃

Contributor

boaz1337 commented Mar 11, 2017

indeed, i am 馃

Hide command options that are related to Windows
Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>
@dnephin

LGTM

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

This comment has been minimized.

@dnephin

dnephin Mar 13, 2017

Member

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.

@dnephin

dnephin Mar 13, 2017

Member

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 馃

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 31598 has succeeded
Details
janky Jenkins build Docker-PRs 40220 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 300 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 11299 has succeeded
Details
z Jenkins build Docker-PRs-s390x 181 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.04.0 milestone 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
Hide command options that are related to Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment