Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #37: Attempting to back up to a remote destination generates exception #38

Merged
merged 1 commit into from
Mar 20, 2016
Merged

Issue #37: Attempting to back up to a remote destination generates exception #38

merged 1 commit into from
Mar 20, 2016

Conversation

glennlagasse
Copy link

This fixes the issue with generating snapshots on a remote host

@seanh
Copy link
Owner

seanh commented Mar 17, 2016

This looks good @glennlagasse, I'll get it reviewed and merged as soon as I can. It will need a couple of tests written. Thanks

@glennlagasse
Copy link
Author

Sounds good @seanh. I'll take a look at the existing tests and see what can be added. Did you have anything particular in mind? Cheers.

@seanh
Copy link
Owner

seanh commented Mar 18, 2016

I think what I'd like to see are just unit tests for _ls_snapshots() that test that it does the right thing when the destination is local and when it's remote. You probably want to use the mock library to patch the _run() function and the os module and test that _ls_snapshots() makes the correct calls to those modules for different inputs. And also test that it returns the correct list of snapshots, of course.

By the way, what is "/*/"?

Can you replace d = d.rsplit('/', 1) with os.path.separator or with a call to the os.path module's functions for splitting paths?

There's actually an interesting problem here that it won't work when the path separator used by the remote operating system is different than the one used by the local platform, but that can be an issue to solve another time.

Thanks!

@seanh seanh merged commit 044d71c into seanh:master Mar 20, 2016
@seanh
Copy link
Owner

seanh commented Mar 20, 2016

@glennlagasse I've tidied this up a bit, added tests, and released it as version 1.0.4. pip install --upgrade snapshotter should install it for you. Thanks!

I did discover a couple more issues related to remote hosts, while testing this, these are both just annoying and can be worked around but I'd like to get them fixed one day: #40, #16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants