feat: support multiple source files in mgrctl cp (Fixes #657)#734
feat: support multiple source files in mgrctl cp (Fixes #657)#734YatesMold wants to merge 6 commits into
Conversation
YatesMold
force-pushed
the
feat-mgrctl-cp-multi-source
branch
from
ec2d25d to
16cbd69
Compare
cbosdo
left a comment
cbosdo
left a comment
There was a problem hiding this comment.
The PR looks quite good to me. I left some suggestions to make the help clearer, they may not be perfect, but it can't be left as is for sure.
The copyright year is not updated: add a copyright line with your name on the file you changed.
It would be probably nice to check that we don't have a mix of path starting with server: and not in the sources.
You also need to add a changelog file using mkchlog. see the wiki contributing page
I would also need the PR description to be in the git commit message: it makes it easier to dig in code in the future.
|
|
||
| cpCmd := &cobra.Command{ | ||
| Use: "cp [path/to/source.file] [path/to/destination.file]", | ||
| Use: "cp [path/to/source1.file] [path/to/source2.file ...] [path/to/destination]", |
There was a problem hiding this comment.
| Use: "cp [path/to/source1.file] [path/to/source2.file ...] [path/to/destination]", | |
| Use: "cp [path/to/source1.file ...] [path/to/destination]", |
| Long: L(`Takes a source and destination parameters. | ||
| One of them can be prefixed with 'server:' to indicate the path is within the server pod.`), |
There was a problem hiding this comment.
| Long: L(`Takes a source and destination parameters. | |
| One of them can be prefixed with 'server:' to indicate the path is within the server pod.`), | |
| Long: L(`Takes at lease one source and destination parameters. | |
| Several sources files can be passed, the last parameter will be the destination. | |
| Either the sources parameters or the destination on can be prefixed with 'server:' to indicate the path is within the server pod.`), |
…#657) Allow passing multiple source file paths to `mgrctl cp`. The last argument is treated as the destination. All sources must be consistently either local paths or server: prefixed paths; mixing is not allowed. Update the Use string and Long description to reflect the new multi-source behavior. Add validation to reject mixed local and server source paths.
YatesMold
force-pushed
the
feat-mgrctl-cp-multi-source
branch
from
16cbd69 to
3735e3b
Compare
|
Hi @cbosdo, I've addressed all the review feedback: fixed the Use string and Long description, added a copyright line, added validation to reject mixed local/server sources, created a changelog entry, and expanded the commit message with the PR description. Please take another look when you get a chance |
|
2 participants
What does this PR change?
Support multiple source files in
mgrctl cpby acceptingMinimumNArgs(2)instead ofExactArgs(2), treating the last argument as the destination, and looping through all preceding arguments to copy each one.Example:
mgrctl cp a b server:/root/now works instead of requiring a shell loop.Test coverage
Links
Issue(s): #657
Changelogs