-
Notifications
You must be signed in to change notification settings - Fork 645
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
migrate bbolt info command to cobra style #636
Conversation
0e755b6
to
e062784
Compare
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.
Hey @charles-chenzz - Thanks for your work on this. Please resolve test failures detected by make fmt
.
96c8323
to
7cdde7a
Compare
please ignore, I'll take a look on it |
7cdde7a
to
f49d428
Compare
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.
Thanks for addressing the fmt
errors @charles-chenzz. I've reviewed the changes, please see two suggestions below.
infoCmd := &cobra.Command{ | ||
Use: "info PATH", | ||
Short: "Info prints basic information about the Bolt database at PATH.", | ||
RunE: func(cmd *cobra.Command, args []string) error { |
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.
Suggest validating args cobra style, if I don't supply the DB path currently and just run bbolt info
all I get is a panic and it's unclear from a user perspective what was done wrong.
Example arg validation:
bbolt/cmd/bbolt/command_surgery.go
Lines 57 to 65 in cd4a757
Args: func(cmd *cobra.Command, args []string) error { | |
if len(args) == 0 { | |
return errors.New("db file path not provided") | |
} | |
if len(args) > 1 { | |
return errors.New("too many arguments") | |
} | |
return 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 think this can use
Args: cobra.ExactArgs(1)
but I've use the example that you list
Signed-off-by: charles-chenzz <[email protected]>
f49d428
to
8d7ebe6
Compare
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 - Thanks @charles-chenzz
Discussed during sig-etcd triage meeting. This would be a breaking change, we will hold merging this until we release 1.4.0. |
it's ok. Thanks for clarifying this! @jmhbnz |
#472
migrate bbolt info to cobra style