add rsync flag option to copy files using rsync #3143
Conversation
olamilekan000
force-pushed
the
add-rsycn-as-an-option-for-copying-files-btw-host-and-guest
branch
6 times, most recently
from
a508328 to
4b57bc6
Compare
There was a problem hiding this comment.
Do we have rsync in the guest in all cases? Minimal vm images do not have rsync.
Also adding options is bad both for users and for testing. If it is better to copy with rsync, we should detect if rsync is available in the guest and host, and use it without adding new option. Otherwise fall back to scp.
If we always have rsync, better to use it instead of scp and minimize the testing matrix.
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
This duplicate the lookup code. We could do:
tool := "scp"
if useRsync {
tool = "rsync"
}
arg0, err = exec.LookPath(tool)
if err != nil {
return err
}
There was a problem hiding this comment.
Yeah, that makes sense. I was only following the discussion in the issue. I thought there was a consensus to go with having rsyn as a flag.
cmd/limactl/copy.go
Outdated
| } else { | ||
| // for scp, append SSH options and scpArgs | ||
| cmdArgs = append(cmdArgs, append(sshArgs, scpArgs...)...) | ||
| } |
There was a problem hiding this comment.
This change complicates the code by adding useRsync condition on every step. This is not maintainable.
A better way to do this is to add a function to create a scp command arguments, and a function to create rsync command arguments. Then use shared code to run the command.
cmd/limactl/copy.go
Outdated
| @@ -67,6 +82,9 @@ func copyAction(cmd *cobra.Command, args []string) error { | |||
|
|
|||
| if verbose { | |||
| scpFlags = append(scpFlags, "-v") | |||
| if useRsync { | |||
| scpFlags = append(scpFlags, "--progress") | |||
There was a problem hiding this comment.
We cannot use variable scpFlags to keep rsync flags. This will confuse the next developer. Renaming the variable to toolFlags will avoid this.
cmd/limactl/copy.go
Outdated
| @@ -34,6 +34,7 @@ func newCopyCommand() *cobra.Command { | |||
|
|
|||
| copyCommand.Flags().BoolP("recursive", "r", false, "copy directories recursively") | |||
| copyCommand.Flags().BoolP("verbose", "v", false, "enable verbose output") | |||
| copyCommand.Flags().BoolP("rsync", "", false, "use rsync for copying instead of scp") | |||
There was a problem hiding this comment.
This is not flexible enough. What if we want to another tool later? If we add an option, adding a "tool" or "backend" option will be better, with possible values "scp", "rsync".
There was a problem hiding this comment.
the tool option will default to rsync right?
There was a problem hiding this comment.
If we add an option rsync sounds like a better default. But this is trivial to change if needed.
There was a problem hiding this comment.
Let's just remove the flag and only support rsync, unless there is a use case where scp is preferable over rsync
|
Also, do we have tests for code modified in this PR? if not we should not change it before adding tests. The new rsync flow should also have tests. |
|
@olamilekan000 I've asked before to please test your changes locally instead of force-pushing an update every 10 minutes for 2 hours, triggering potentially costly CI runs all the time. |
I'd appreciate it if you can help with steps to run locally. I did try but It didn't work as expected. |
Yes, rsync is always available on macOS and most Linux distros. |
@olamilekan000, Can you describe how did you try to test and why it did not work well? You can also join the #lima slack channel to get quicker help from other developers. |
olamilekan000
force-pushed
the
add-rsycn-as-an-option-for-copying-files-btw-host-and-guest
branch
from
4b57bc6 to
aa5cf22
Compare
@nirs I have joined and shared my error logs on the channel. |
Signed-off-by: olalekan odukoya <[email protected]>
olamilekan000
force-pushed
the
add-rsycn-as-an-option-for-copying-files-btw-host-and-guest
branch
from
aa5cf22 to
4c95f10
Compare
issue #2198