-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix: Support multiple --logger
flags
#704
Conversation
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.
Looked up the docs to support this while working on it. Meant to do it once I got the one working, thanks for taking care of.
LGTM
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! Just one non-blocking suggesting.
cli/root.go
Outdated
"Named logger parameter override. usage: --logger <name>,level=<level>,output=<output>,...", | ||
rootCmd.PersistentFlags().StringArray( | ||
"logger", []string{}, | ||
"Named logger parameter override. Usage: --logger <name>,level=<level>,output=<output>,...", |
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.
suggestion: Perhaps similar to fred's suggestion to above we can make it clear that can specify multiple --logger
flags in Usage section.
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've changed slightly the text. By default, cobra indicates the expected type (here stringArray
...), so there is the information for the user. Because of these two reasons, I suggest that it is sufficiently clear that it can be used multiple times.
That said I'm not satisfied with the type name that is shown as it could be clearer.
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.
Minor suggestions. Otherwise LGTM!
8221578
to
3e59adf
Compare
Relevant issue(s)
Resolves #699
Description
Support multiple
--logger
flagsTasks
How has this been tested?
Manual
Specify the platform(s) on which this was tested: