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.

Sign up for GitHub

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

Jump to bottom

Add a "top-level" annotation to hide persistent flags #1106

Merged
merged 1 commit into from
Jun 1, 2018
Merged

Add a "top-level" annotation to hide persistent flags #1106

merged 1 commit into from
Jun 1, 2018

Conversation

silvin-lubecki
Copy link
Contributor

- What I did
I added a "top-level" annotation to hide persistent flags in all sub-commands, excepting some specific commands, while printing help.
Fixes issue #1099.

- How to verify it

# All commands excepting stack and version hides orchestrator flag
$ docker container cp --help

Usage:  docker container cp [OPTIONS] CONTAINER:SRC_PATH DEST_PATH|-
        docker cp [OPTIONS] SRC_PATH|- CONTAINER:DEST_PATH

Copy files/folders between a container and the local filesystem

Options:
  -a, --archive       Archive mode (copy all uid/gid information)
  -L, --follow-link   Always follow symbol link in SRC_PATH

$ docker version --help
...
      --orchestrator string   Orchestrator to use (swarm|kubernetes|all)
...

$ docker stack --help
...
      --orchestrator string   Orchestrator to use (swarm|kubernetes|all)
...

# Works also with stack sub-commands
$ docker stack ls --help
...
      --orchestrator string   Orchestrator to use (swarm|kubernetes|all)
...

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

image

@silvin-lubecki
Added a "top-level" annotation to hide persistent flags in all sub-co…
c003ed1 
…mmands, excepting some specific commands, while printing help

Fixes issue docker#1099

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
vdemeester
vdemeester approved these changes Jun 1, 2018
View reviewed changes
Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

mat007
mat007 reviewed Jun 1, 2018
View reviewed changes
cmd/docker/docker.go
@@ -245,6 +246,12 @@ func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) {
if !isOSTypeSupported(f, osType) || !isVersionSupported(f, clientVersion) {
f.Hidden = true
}
// root command shows all top-level flags
if cmd.Parent() != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it the default is false for f.Hidden, therefore --orchestrator is displayed by docker --help (i.e. when cmd.Parent() is nil), is this what we want?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's the desired behavior, it's possible to set it as global flag (i.e., docker --orchestrator=foo stack deploy), but when showing help for (e.g.) docker stack deploy we show it as a flag for that command, so: docker stack deploy --orchestrator=foo

Is that what you meant?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I find it a bit odd that it can be set as a global flag given that it then has no effect and is ignored for most of the sub-commands, however it's out of the scope of this PR anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of the top level options as well, although they can be handy for making shell aliases (alias dockerk -> docker --orchestrator=foo --host=bla)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that's what we want, so "top level" flags are shown in the root command help (docker --help), hidden in all sub commands help but still available, excepting the one declared in the annotations.

thaJeztah
thaJeztah approved these changes Jun 1, 2018
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It's a bit quirky, but we can always find a better solution.

In future most commands will support orchestrator, so probably becomes less of an issue

@thaJeztah
Copy link
Member

Merging, but @mat007 let me know if there's still something we should address

@thaJeztah thaJeztah merged commit 2014e99 into docker:master Jun 1, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.06.0 milestone Jun 1, 2018
@silvin-lubecki silvin-lubecki deleted the fix-orchestrator-flag-persistent branch June 4, 2018 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mat007 mat007 mat007 left review comments

@thaJeztah thaJeztah thaJeztah approved these changes

@vdemeester vdemeester vdemeester approved these changes

@vieux vieux Awaiting requested review from vieux

@dnephin dnephin Awaiting requested review from dnephin

Assignees
No one assigned
Labels
priority/P1 status/2-code-review
Projects
None yet
Milestone
18.06.0
Development

Successfully merging this pull request may close these issues.

The new --orchestrator flag is showing up on too many commands
5 participants
@silvin-lubecki @thaJeztah @vdemeester @mat007 @GordonTheTurtle