-
Notifications
You must be signed in to change notification settings - Fork 2k
Add a "top-level" annotation to hide persistent flags #1106
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
Add a "top-level" annotation to hide persistent flags #1106
Conversation
…mmands, excepting some specific commands, while printing help Fixes issue docker#1099 Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
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 🎉
| f.Hidden = true | ||
| } | ||
| // root command shows all top-level flags | ||
| if cmd.Parent() != nil { |
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 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?
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.
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?
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.
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.
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.
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)
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.
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.
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. 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
|
Merging, but @mat007 let me know if there's still something we should address |

Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.

- 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
- Description for the changelog
--orchestratorflag is showing up on too many commands #1099)- A picture of a cute animal (not mandatory but encouraged)