Faster SCP Copying for Local to Remote directories#18
Conversation
1. If it does, assume we are doing local to remote (as the first argument would use the 'sess' header otherwise. 2. DRY up the Copy arg to Scp args so we can prune the remote dirpath in a way that's re-usable, there's no guarantee that's the first SCP arg, but there is a guarantee it is the second one post validation.
cmd/copy.go
Outdated
| } else { | ||
| err := copySourceSCP(scpArgs[0], scpArgs[1], publicKeyPath) | ||
| if err != nil { | ||
| ui.Infof("❎ Unsuccessful copy %s => %s", scpArgs[0], scpArgs[1]) |
cmd/copy.go
Outdated
| err := copySourceSCP(scpArgs[0], scpArgs[1], publicKeyPath) | ||
|
|
||
| // If the first argument of the SCP args points towards a logical directory assume local => remote | ||
| pathInfo, _ := os.Stat(scpArgs[0]) |
There was a problem hiding this comment.
This shouldn't ignore the error. The dereference below will cause a panic if the file doesn't exist. Also, we probably shouldn't assume the direction. We know that the remote path will always be prefixed by sess:. Let's use that info to determine the direction.
There was a problem hiding this comment.
Yeah I'll clean this up, it's a bit hacky
| } | ||
|
|
||
| func isLocalDirToRemoteCopy(args []string) bool { | ||
| if strings.Contains(args[0], "sess:") { |
There was a problem hiding this comment.
This should not access slice elements without checking the length. This will cause a panic if the slice is empty. This is why I think it's better not to break logic up too much.
The args check should happen in the cobra command handler where we can be sure the slice has an expected length. We should extract the args from the slice there and pass them to this function as a string.
| if strings.Contains(args[1], "sess:") { | ||
| return false | ||
| } | ||
|
|
||
| pathInfo, err := os.Stat(args[1]) |
There was a problem hiding this comment.
Same issue as above accessing slice elements. This it prone to panics. The args parsing should happen in the cobra handler entry points.
| exec, err := getTargetExec(cmd, args) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if exec == nil { | ||
| return fmt.Errorf("At least one remote host must be specified") | ||
| } | ||
| if exec.Connection == nil { | ||
| return fmt.Errorf("Target session must have an active connection") | ||
| } | ||
| if exec.SSHKey.PublicKey == nil && config.SSHPublicKeyPath == "" { | ||
| return fmt.Errorf("Failed to identify public key, check your Unweave config file or specify it manually") | ||
| } | ||
|
|
||
| scpArgs, err := formatPaths(exec, args) | ||
| if err != nil { | ||
| ui.Infof("❌ Unsuccessful copy: %s", err.Error()) | ||
| } | ||
| ui.Infof(fmt.Sprintf("🔄 Copying %s => %s", scpArgs[0], scpArgs[1])) | ||
|
|
||
| switch { | ||
| case isLocalDirToRemoteCopy(args): |
There was a problem hiding this comment.
The args should be pars in this function and not passed to handlers as a slice. Here, we know the exact number of args expected because cobra validated them for us. If you pass them down to smaller private functions as a slice, we lose the information about the slice length. Another function could later call the isLocalDirToRemoteCopy function with an empty slice an cause a panice.
Instead, something like this is much safer and clearer to understand:
isLocalDirToRemoteCopy(pathA, pathB string)
This PR ensures we use the archiving process when copying directories from the local host to remote.