Allow copying files from remote host(s) to local host #47

Caid11 · 2015-11-05T01:31:10Z

Adds support for copying files from remote hosts to local host as described in #40.

A new method, copy_file_to_local, has been added to both pssh_client and ssh_client. In ssh_client, its logic is pretty similar to copy_file, but was different enough that I figured it would be better to create a new method rather than make copy_file more complex. In pssh_client, the logic is almost identical between copy_file and copy_file_to_local; the method was duplicated to maintain the pattern between ssh_client and pssh_client.

While I think keeping copy_file simple and creating a new method is better, I also see the benefit of using one method to perform both functions, and am completely open to rolling the two functions together.

Filenames are de-duplicated by appending an underscore followed by the hostname when using pssh_client to copy files to the local host.

…ient.

… to scp a directory.

…g test.

pkittenis · 2015-11-05T11:42:28Z

pssh/ssh_client.py

+        if local_file.startswith(os.path.sep):
+            destination = os.path.sep + destination
+        if not os.path.exists(destination):
+            os.mkdir(destination)


os.mkdir is not recursive and will fail if parent path(s) do not exist.

Suggest os.makedirs is used instead.

Also needs exception handling.

pkittenis · 2015-11-05T11:52:02Z

Many thanks for picking up another feature enhancement :)

Comments in line.

Caid11 · 2015-11-05T14:59:03Z

No problem! Thanks for your patience and feedback as I take this on.

pkittenis · 2015-11-05T17:36:57Z

pssh/pssh_client.py

+                                                password=self.password,
+                                                port=self.port, pkey=self.pkey,
+                                                forward_ssh_agent=self.forward_ssh_agent)
+        return self.host_clients[host].copy_file_to_local(remote_file, local_file + '_' + host)


Though there is little performance gained by it in this case, given the small memory footprint of the variables, this is better written as '_'.join([local_file, host])

For very large host lists, it can make a pretty big difference.

Didn't realize using + to concatenate strings did that. I'll switch it to join.

pkittenis · 2015-11-06T17:08:29Z

pssh/ssh_client.py

+            try:
+                os.makedirs(destination)
+            except OSError, exception:
+                logger.error("Unable to create local directory structure.")


Would be good to log the directory the error occurred in - OSError exception does not include that info.

Also, to re-raise the exception that was caught in a try:, can just use raise by itself within the try block. No need to hold the exception variable.

Thanks for the tip! Will get that fixed.

pkittenis · 2015-11-06T17:52:17Z

Out of interest, are you set on the copy_file_to_local name?

I agree the remote copy behaviour should be in separate function - they are two separate features.

Would favour a name such as copy_remote_file though.

In my view it's more concise and clearer to the casual reader what copy_file and copy_remote_file do, especially given the context the arguments give. copy_file takes local and remote paths, to and from, copy remote file takes remote and local paths, to and from.

Prepared to be convinced otherwise though :)

pkittenis · 2015-11-06T17:54:23Z

pssh/pssh_client.py

+                for host in self.hosts]
+
+    def _copy_file_to_local(self, host, remote_file, local_file):
+        """Make sftp client, copy file to local"""


This docstring is not valid for this function, should be in the SSHClient equivalent function.

NB - docstrings for internal functions (ones that start with _) are completely optional as they will not appear in the documentation, only in, say, ipython interactive documentation.

Thanks for the note on internal vs. external docstrings, I didn't realize the internal ones wouldn't show up in the docs.

I am a little confused on why the docstring is invalid, however. I attempted to just change the docstring for copy_file from pssh_client to reflect copy_file_to_local. Are you saying I should put it in ssh_client instead?

Yes, that's right - the 'make sftp client' bit of the docstring applies to SSHClient._copy_file but not ParallelSSHClient._copy_file.

That said, not that important since they are both internal functions.

Ah, I see. Thanks for clearing that up for me.

Caid11 · 2015-11-06T21:21:06Z

I initially thought about going with copy_remote_file, but decided not to because both functions involve remote files, just in different ways. However, I didn't think about how the arguments give the name more context. With that in mind, I agree that copy_remote_file would be a better name.

Caid11 · 2015-11-18T17:45:34Z

Just a heads up @pkittenis, this PR isn't dead, I've just been a little too busy to work on it the last little while.

pkittenis · 2015-11-25T12:10:15Z

No worries, thanks for the heads up

pkittenis · 2015-12-02T18:32:08Z

Pro tip for when you get a chance - if you add the email address used in the commits that you have made to your github account, your username will show up as contributor in this project 👍

Contributions, yours and others, are much appreciated and there should be recognition of them :)

Caid11 · 2015-12-03T03:16:13Z

Wow, I had no idea. Thanks! Also, thanks for your patience while I've let this sit. I've got finals in just a couple of weeks, then I'll be able to get this finished and hopefully make some more contributions.

…to copy_remote_local Conflicts: pssh/pssh_client.py pssh/ssh_client.py Merge fork with upstream.

…sage

…uplicated.

Caid11 · 2015-12-23T18:03:53Z

Think I've gotten this to a good point. Can't figure out to force os.mkdir to fail, though, so I'm having some trouble writing that test.

pkittenis · 2016-01-14T18:52:50Z

Cool, thank you, I'll have a look at it soon.

pkittenis · 2016-05-03T17:07:47Z

Re-raised at #58 with upstream changes.

Kincaid Savoie added 4 commits November 4, 2015 08:44

Added function to copy files from remote host to local host in ssh_cl…

b0ff89a

…ient.

Added statement to yell at the user if they don't set recurse and try…

d8a59e9

… to scp a directory.

Added tests for copy_file_to_local.

704f86f

Implemented copy_file_to_local in pssh_client and created accompanyin…

ccc7482

…g test.

pkittenis reviewed Nov 5, 2015
View reviewed changes

Switch mkdir to makedirs and surround it with a try/catch.

ccc64f7

pkittenis reviewed Nov 5, 2015
View reviewed changes

Kincaid Savoie added 2 commits November 5, 2015 13:49

Broke parent_path_split into its own method.

99f87c9

Switched from "+" for concatenation to "join" for performance.

480788e

pkittenis reviewed Nov 6, 2015
View reviewed changes

Kincaid Savoie and others added 7 commits December 22, 2015 18:13

Merge branch 'master' of https://github.com/pkittenis/parallel-ssh in…

00989a8

…to copy_remote_local Conflicts: pssh/pssh_client.py pssh/ssh_client.py Merge fork with upstream.

Removed unnecessary variable from raised exception; updated error mes…

3332213

…sage

Let recurse get passed through from the pssh client.

71d4559

Fixed _parent_path_split method and included it where some code was d…

f8042b2

…uplicated.

Removed unneccessary conditional.

7dbce34

Added test to ensure remote directories can be copied to the local host.

4fb5935

Fixed tests so they actually clean up after themselves.

a516772

pkittenis mentioned this pull request May 3, 2016

Allow copying files from remote host(s) to local host #58

Merged

pkittenis closed this May 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow copying files from remote host(s) to local host #47

Allow copying files from remote host(s) to local host #47

Caid11 commented Nov 5, 2015

pkittenis Nov 5, 2015

pkittenis commented Nov 5, 2015

Caid11 commented Nov 5, 2015

pkittenis Nov 5, 2015

Caid11 Nov 5, 2015

pkittenis Nov 6, 2015

Caid11 Nov 6, 2015

pkittenis commented Nov 6, 2015

pkittenis Nov 6, 2015

Caid11 Nov 6, 2015

pkittenis Nov 10, 2015

Caid11 Nov 10, 2015

Caid11 commented Nov 6, 2015

Caid11 commented Nov 18, 2015

pkittenis commented Nov 25, 2015

pkittenis commented Dec 2, 2015

Caid11 commented Dec 3, 2015

Caid11 commented Dec 23, 2015

pkittenis commented Jan 14, 2016

pkittenis commented May 3, 2016

Allow copying files from remote host(s) to local host #47

Allow copying files from remote host(s) to local host #47

Conversation

Caid11 commented Nov 5, 2015

Choose a reason for hiding this comment

pkittenis commented Nov 5, 2015

Caid11 commented Nov 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkittenis commented Nov 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Caid11 commented Nov 6, 2015

Caid11 commented Nov 18, 2015

pkittenis commented Nov 25, 2015

pkittenis commented Dec 2, 2015

Caid11 commented Dec 3, 2015

Caid11 commented Dec 23, 2015

pkittenis commented Jan 14, 2016

pkittenis commented May 3, 2016