OndrejHome / fast-vm

'fast-vm' is a script for defining VMs from images provided in thin LVM pool.
GNU General Public License v3.0
22 stars 14 forks source link

Fix scp with multiple file paths #42

Closed itsbill closed 6 years ago

itsbill commented 6 years ago

Hi,

After a bit more use I've found myself trying commands like fast-vm scp 71 Downloads/*.rpm vm: and that currently fails to copy. It returns quickly, having copied nothing.

This PR loops through the filenames, transforming "vm:" if found. I can now successfully scp multiple files to and from one or multiple VMs. Comments welcome, and thanks in advance!

OndrejHome commented 6 years ago

Hi @itsbill , Thank you for submitting another PR.

Adding ability to scp to handle also '*' and possible multiple files sounds good to me but I have some notes to proposed implementation.

What about just taking all the arguments and making them into form that contains the needed IP addresses of VMs with some sed magic. For inspiration when I was making fast-vm to operate on multiple VMs I have passed the arguments in the following way and I think it might be also possible here. What do you think?

https://github.com/OndrejHome/fast-vm/blob/455b0ff83927697fbd96b592d07ff754e05750e8/fast-vm#L167

Additionally I would like to keep so far fast-vm without dependency on bash in Debian (supported from 1.4) so there is need to avoid bash-only things like bash arrays. I use checkbashism to avoid these things.

Would you be able to have a look and try some other implementation?

NOTE: We would need to update the fast-vm.bash_completion and man pages once this is done to reflect the changes.

itsbill commented 6 years ago

HI @OndrejHome thank you for the quick review! I should not write diffs at the end of a long day... please see this version instead. Thank you again.

OndrejHome commented 6 years ago

Hi @itsbill, Thank you for revision. It works for the * but I have tried the syntax with {} brackets that I use sometimes and the it fails when that is used in the remote part. For example:

./fast-vm scp 61 vm:{a,b,c}-test /tmp
[61][inf] checking the 192.168.34.61 for active SSH connection (ctrl+c to interrupt)
[61][inf] 
[61]SSH ready
ssh: Could not resolve hostname vm: Name or service not known
ssh: Could not resolve hostname vm: Name or service not known
Warning: Permanently added '192.168.34.61' (ECDSA) to the list of known hosts.
c-test 

Problem seems to be that only one occurrence of vm: is replaced with actual IP address of the VM. So I have tried following change which seems to address this.

shift 2
arguments=$(echo "$@"|sed "s/vm:/root@192.168.$SUBNET_NUMBER.$vm_number:/g")

Above will replace all occurrences (g tells to not stop after first match) of vm: with that VM IP address and then even the syntax with {} brackets will work.

./fast-vm scp 61 vm:{a,b,c}-test /tmp
[61][inf] checking the 192.168.34.61 for active SSH connection (ctrl+c to interrupt)
[61][inf] 
[61]SSH ready
Warning: Permanently added '192.168.34.61' (ECDSA) to the list of known hosts.
a-test                                                                                                                                                                  100%    0     0.0KB/s   0.0KB/s   00:00    
Warning: Permanently added '192.168.34.61' (ECDSA) to the list of known hosts.
b-test                                                                                                                                                                  100%    0     0.0KB/s   0.0KB/s   00:00    
Warning: Permanently added '192.168.34.61' (ECDSA) to the list of known hosts.
c-test 

Could you please check the updated syntax with your use cases and if that fits your use case update the code so I can merge it? (I will take care of changing man page and bash completion if necessary) If you have any questions to above feel free to ask.

itsbill commented 6 years ago

Hi @OndrejHome I've redone the commit, but this adds \b to the regex so that we don't match vm: when it is part of a filename. With this diff, I can scp with * and with {}. Thanks!

OndrejHome commented 6 years ago

Thank you @itsbill! Looks good, merging.