-
Notifications
You must be signed in to change notification settings - Fork 500
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
[feat] tools-v2: add snapshot copyset --all #2618
Conversation
cicheck |
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.
great job~ keep coding~
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
@Cyber-SiKu please take a look this pr~ |
cicheck |
@@ -488,6 +488,10 @@ func AddBsAllOptionFlag(cmd *cobra.Command) { | |||
AddBsBoolOptionFlag(cmd, CURVEBS_ALL, "all") | |||
} | |||
|
|||
func AddBsCopysetIdOptionFlag(cmd *cobra.Command) { | |||
AddBsUint32OptionFlag(cmd, CURVEBS_COPYSET_ID, "copyset id") |
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.
option flag should be given default value
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.
done
@@ -56,65 +59,107 @@ func (sRpc *SnapshotRpc) Stub_Func(ctx context.Context) (interface{}, error) { | |||
return sRpc.Client.Snapshot(ctx, sRpc.Request) | |||
} | |||
|
|||
type SnapshotOneCommand struct { | |||
type SnapshotAllRpc struct { |
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.
You can split snapshotAllRpc and snapshotRpc into two commands, and then remove these two commands separately, which will make it 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.
done
31c74bd
to
dd6b5b0
Compare
cicheck |
@Cyber-SiKu a big change waits for your cr. |
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.
wow~ using sub-command is a good way to let user understand our code!
cicheck |
retErr.Format(err.Error()) | ||
return nil, nil, retErr | ||
} | ||
return sCmd.TableNew, sCmd.Result, cmderror.Success() |
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.
You can return the response (or a map) directly instead of the table, the table has no information
retErr.Format(err.Error()) | ||
return nil, nil, retErr | ||
} | ||
return sCmd.TableNew, sCmd.Result, cmderror.Success() |
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.
ditto
response, err := basecmd.GetRpcResponse(sCmd.Rpc.Info, sCmd.Rpc) | ||
sCmd.Error = err | ||
func (sCmd *SnapshotCopysetCommand) RunCommand(cmd *cobra.Command, args []string) error { | ||
var table, res interface{} |
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.
Put the processing of the result here
Signed-off-by: montaguelhz <1443171175@qq.com>
cicheck |
What problem does this PR solve?
Issue Number: #2352
Problem Summary:
What is changed and how it works?
What's Changed:
How it Works:
curve bs snapshot copyset --all
Side effects(Breaking backward compatibility? Performance regression?):
Check List