-
Notifications
You must be signed in to change notification settings - Fork 526
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 bs recover volume #2936
Conversation
a193cc0
to
bb2857d
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.
Rebase commits as one. If you open a new pr, plz close the old one
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.
Is this file unused?
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.
the file is used, which wraps the RecoverFile RPC.
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.
One volume.go file is ok.
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.
One volume.go file is ok.
Actually, my first PR just did so, then I'm told that recover is a middle comamnd, maybe it would recover something else in the future? So I add this mid command. I think it's innocuous.
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, The recover is seen as middle command but you can refer the stop snapshot
command.
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, The recover is seen as middle command but you can refer the
stop snapshot
command.
You're right! I finally got what you mean and make correspoding change. Please take a look, thanks.
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, but it also can be realized using one file and refer to other command.
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.
Sorry, I don't catch what you mean. I just realize it using one file, Is there something still need to improve?
927d463
to
48b6138
Compare
@caoxianfei1 PTAL |
tools-v2/README.md
Outdated
+---------+ | ||
| RESULT | | ||
+---------+ | ||
| 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.
maybe we can show path in table.
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.
func NewRecoverFileCommand() *RecoverFileCommand { | ||
RecoverFileCommand := &RecoverFileCommand{ | ||
FinalCurveCmd: basecmd.FinalCurveCmd{}, | ||
} | ||
basecmd.NewFinalCurveCli(&RecoverFileCommand.FinalCurveCmd, RecoverFileCommand) | ||
return RecoverFileCommand | ||
} |
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 func on top.
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.
9ce7daf
to
5a4c6bd
Compare
d34076c
to
89f1287
Compare
cicheck |
1 similar comment
cicheck |
@CrystalAnalyst need rebase otherwise ci may fail. |
f95d183
to
7d4dafd
Compare
cicheck |
2 similar comments
cicheck |
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.
LGTM!
@CrystalAnalyst please fix the conflict. |
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.
good job
return recoverCommand.Error.ToError() | ||
} | ||
out := make(map[string]string) | ||
out[cobrautil.ROW_FILE_NAME] = *recoverCommand.Rpc.Request.FileName |
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.
it might be handled in Init
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
|
||
var _ basecmd.FinalCurveCmdFunc = (*RecoverFileCommand)(nil) | ||
|
||
func (recoverCommand *RecoverFileCommand) Init(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.
why not rCmd
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
7d4dafd
to
e24428d
Compare
Use |
Also, it might be better to leave a blank line between the comment and the code. |
4ef46ea
to
328d977
Compare
okay. |
yes, I reset to previous commit and use rebase solved the conflict justnow. |
328d977
to
ef54fb6
Compare
the commit message suggestion is: |
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.
LGTM
) | ||
|
||
const ( | ||
RecoverExample = `curve bs recover volume --path /curvebs-volume-path |
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.
const (
RecoverExample = `curve bs recover volume --path /curvebs-volume-path
--user username [--password password] [--fileid fileid]`
)
Extra Spaces may appear when executing help
command.
cicheck |
Signed-off-by: kevin <[email protected]>
ef54fb6
to
3491478
Compare
cicheck |
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
cicheck |
All checks have passed. Is there anyone who is authorized to help me merge this pull request ? @caoxianfei1 @montaguelhz @baytan0720 I'd appreciate that, thanks. |
What problem does this PR solve?
Issue Number: #2588
Problem Summary: add recover as a middle command, and it has sub-command volume.
What is changed and how it works?
What's Changed: add recover middle command directory, and subcommand directory volume.
How it Works: recover.go is the middle command. Inside the sub-direcotry volume , the volume.go is the final command, while the recover.go wraps the RecoverFilePRC to execute the real recover procedure.
Side effects(Breaking backward compatibility? Performance regression?):
Check List