-
Notifications
You must be signed in to change notification settings - Fork 254
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
cmd: improve command usage message by grouping related flags #443
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rolinh
added
⌨️ area/cli
Impacts the command line interface of any command in the repository.
release-note/minor
This PR introduces functionality that users may find relevant to operating Hubble.
labels
Dec 3, 2020
glibsm
reviewed
Dec 3, 2020
As part of refactoring work to improve commands help message output, it is useful to be able to get the name of a given `pflag.FlagSet`. In the latest released version (v1.0.5, released 2019.09.18), this information is not available as the `FlagSet` name, although provided by the caller, is a private field. However, the commit that was pushed immediately after v1.0.5 adds a `Name()` method to FlagSet, which comes in handy (see [0] for details). As the difference with the v1.0.5 is low risk and no new releases have been published, update pflag to the commit right after v1.0.5. [0]: spf13/pflag@81378bb Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
I wanted to create multiple commits but it turns out to be complicated. Let's summarize what this commit does: - Add `cmd/common/template` where a `Usage()` can be used by commands to generate a custom template based on the provided `pflag.FlagSets`. This allows grouping related flags when displaying help/usage of a command or subcommand. - With the help of `Usage()` described above, the custom usage function for the `observer` subcommand can be dropped. It was quite hacky and easy to get out of sync by adding a flag to the `observe` command and forgetting to update the usage formatting function to indicate to which flag group the newly added flag belongs. The `observe` command now has several defines several `pflag.FlagSet` for each flag group (selectors, filters, formatting, others). - Global flags (ie flags that are persisted from the root command) have been split into to `pflag.FlagSet` in `cmd/common/config`: `GlobalFlags` and `ServerFlags`. The former contains all global flags (ie: debug and config) and the latter all flags related to connecting to a Hubble server instance. - The global flags section is always printed for each command or subcommand as it apply to any command. However, the new `ServerFlags` flag set needs to be added to the help message by any command or subcommand that makes use of it. For the user, this is really convenient as these flags appear in the help message of a command only when relevant instead of always (eg: the `completion` command does not use the server flags and this does not display help for these when running `hubble completion -h`). The `ServerFlags` are nonetheless global in the sense that they are persistent flags added to the root command. This is done for 2 reasons: not breaking the existing behavior and allowing users to use these flags in any order (e.g. `hubble --server foo observe` also works). - Instead of using hardcoded strings for every configuration key in every command where they need to be used, constants have been defined in `cmd/common/config` and shall be used instead. - Defaults for the configuration directory, fallback configuration directory and configuration file have been moved to `pkg/defaults`. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
rolinh
force-pushed
the
pr/rolinh/refactor-help
branch
from
December 7, 2020 12:40
a4dd55d
to
f9628ae
Compare
gandro
approved these changes
Dec 8, 2020
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.
Awesome! Due the the large size of the PR, it's hard to track what are actual code changes and what just moves code around, but I did try out the branch locally with a bunch of commands and it worked as expected.
glibsm
approved these changes
Dec 8, 2020
maintainer-s-little-helper
bot
added
the
ready-to-merge
This PR has passed all tests and received consensus from code owners to merge.
label
Dec 8, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
⌨️ area/cli
Impacts the command line interface of any command in the repository.
ready-to-merge
This PR has passed all tests and received consensus from code owners to merge.
release-note/minor
This PR introduces functionality that users may find relevant to operating Hubble.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I wanted to create multiple commits but it turns out to be complicated.
Let's summarize what this PR does:
cmd/common/template
where aUsage()
can be used by commands togenerate a custom template based on the provided
pflag.FlagSets
.This allows grouping related flags when displaying help/usage of a
command or subcommand.
Usage()
described above, the custom usage functionfor the
observer
subcommand can be dropped. It was quite hacky andeasy to get out of sync by adding a flag to the
observe
command andforgetting to update the usage formatting function to indicate to
which flag group the newly added flag belongs. The
observe
commandnow has several defines several
pflag.FlagSet
for each flag group(selectors, filters, formatting, others).
been split into to
pflag.FlagSet
incmd/common/config
:GlobalFlags
andServerFlags
. The former contains all global flags(ie: debug and config) and the latter all flags related to connecting
to a Hubble server instance.
subcommand as it apply to any command. However, the new
ServerFlags
flag set needs to be added to the help message by any command or
subcommand that makes use of it. For the user, this is really
convenient as these flags appear in the help message of a command only
when relevant instead of always (eg: the
completion
command does notuse the server flags and this does not display help for these when
running
hubble completion -h
). TheServerFlags
are nonethelessglobal in the sense that they are persistent flags added to the root
command. This is done for 2 reasons: not breaking the existing
behavior and allowing users to use these flags in any order (e.g.
hubble --server foo observe
also works).every command where they need to be used, constants have been defined
in
cmd/common/config
and shall be used instead.directory and configuration file have been moved to
pkg/defaults
.EDIT: sometimes, it's clearer to understand with examples.
This is the help for
observe
, (the grouping of flags is now automatic and based on which flagset the flag is added):This is the help for
config
. Notice that theServer Flags
section is absent.