Teach upload! and download! to respect within(...) #408

sj26 · 2017-10-24T10:45:40Z

I've been using sshkit to automate some deployment tasks recently and found the tasks would be easier if upload! and download! respected my within(...) context.

They already seem to work with remote relative paths which use the remote cwd, but don't respect the within(...) path stack. But we can just prepend pwd_path to remote path if it's relative to make it work.

This might be a breaking change for some users, although the documentation does say to always use absolute paths.

Also I have no idea how to test this, beyond doing a whole bunch of manual testing against a real ssh remote. 😅

sj26 · 2017-10-25T04:44:39Z

I totally missed the functional test directory. After some fun with Vagrant I got some tests similar to the existing tests which upload! into a nested within() structure to prove it all works. And I added a CHANGELOG entry.

mattbrictson · 2017-10-26T04:09:39Z

Thanks for the PR! I appreciate the tests and CHANGELOG entry. I hope to have time to review this soon.

Would you say this is a breaking change?

sj26 · 2017-10-26T04:56:26Z

This doesn't break existing documented usage, which is to use absolute paths. That usage should be totally unaffected.

But if a user has relied on the incidental behaviour of relative paths being relative to the ssh connection's default current working directory (instead of sshkit's within(...) working directory) then that might break. But it was always documented that this wasn't supported, so maybe that's okay? Idk.

Perhaps a minor version increment with a note calling out the change?

mattbrictson

Looks good! If you could add a note regarding potential breakage to the CHANGELOG as you suggested, that would be great. Put it as another bullet point and I'll make sure it gets formatted correctly when I do the next release.

mattbrictson · 2017-10-29T23:01:10Z

lib/sshkit/backends/local.rb

      end

      def upload!(local, remote, options = {})
+        remote = File.join(pwd_path, remote) unless remote[0] == "/"


Nitpick: I'd prefer remote.start_with?("/") instead of remote[0] == "/" here and elsewhere, for readability.

Oh yeah that's much nicer. 👍

I always avoid #start_with? thinking it was ActiveSupport or introduced in Ruby 2+, but it's been around since 1.8.7. 🙈

sj26 · 2017-10-30T03:22:38Z

@mattbrictson Thanks for helping me get this through!

Totally agree with the nitpick, fixed. And I've added a note to the changelog which you can edit as you like. Also had to fix a bundle problem with Ruby 2.0 on Travis.

mattbrictson · 2017-10-30T15:13:07Z

Looks good, thanks!

sj26 · 2017-10-31T03:30:46Z

Thanks! 🙇

When do you think this might make it into a release — is there anything else lined up?

mattbrictson · 2017-11-01T00:30:56Z

This will probably go out on Friday or over the weekend

mattbrictson · 2017-11-03T16:42:31Z

🚢 https://rubygems.org/gems/sshkit/versions/1.15.0

sj26 · 2017-11-08T03:08:46Z

🙇

Fjan · 2017-11-15T14:45:31Z

Just had a script breaking because it relied on the old behaviour. Easy enough to fix, but as far as I can tell simply detecting that we're not in a within block and then keeping the new behaviour should be as easy as adding a check for pwd_path.nil?. So this:
remote = File.join(pwd_path, remote) unless remote.start_with?("/") || pwd_path.nil?
Perhaps we could add that as a refinement of this patch? Or alternatively return a more descriptive error than the current syntax error?

mattbrictson · 2017-11-15T23:03:02Z

@Fjan Ah, you're right. Good catch! This breaks if you upload to a relative remote path without using a within block. Just curious: what is the expected behavior? It just uploads relative to the user's home directory?

Anyway, I agree that a pwd_path.nil? like you suggested is a good sanity check and will fix this bug. Would you be willing to submit a PR?

Fjan · 2017-11-16T07:07:10Z

Yep, the old behaviour just uploads / downloads relative to the user's home directory. I'm a it swamped at work and have not contributed to the project before so it will take some time before I can get to doing a PR, so would be great if someone more experienced can do it.

Fjan · 2017-11-18T09:07:08Z

Ok, I went ahead and created a PR (#411)

@ElvinEfendi

## [1.16.0][] (2018-02-03) * [#417](capistrano/sshkit#417): Cache key generation for connections becomes slow when `known_hosts` is a valid `net/ssh` options and `known_hosts` file is big. This changes the cache key generation and fixes performance issue - [@ElvinEfendi](https://github.com/ElvinEfendi). ## [1.15.1][] (2017-11-18) This is a small bug-fix release that fixes problems with `upload!` and `download!` that were inadvertently introduced in 1.15.0. ### Breaking changes * None ### Bug fixes * [#410](capistrano/sshkit#410): fix NoMethodError when using upload!/download! with Pathnames - [@UnderpantsGnome](https://github.com/UnderpantsGnome) * [#411](capistrano/sshkit#410): fix upload!/download! when using relative paths outside of `within` blocks - [@Fjan](https://github.com/Fjan) ## [1.15.0][] (2017-11-03) ### New features * [#408](capistrano/sshkit#408): upload! and download! now respect `within` - [@sj26](https://github.com/sj26) ### Potentially breaking changes * `upload!` and `download!` now support remote paths which are relative to the `within` working directory. They were previously documented as only supporting absolute paths, but relative paths still worked relative to the remote working directory. If you rely on the previous behaviour you may need to adjust your code.

Resolve relative paths using within() stack

1db04bf

sj26 force-pushed the transfer-with-relative-paths branch from 3b75f57 to 1db04bf Compare October 25, 2017 04:43

mattbrictson reviewed Oct 29, 2017

View reviewed changes

sj26 added 3 commits October 30, 2017 11:47

Embrace String#start_with?

e715e05

Add a note about potential breakage

e105d43

Fix Ruby 2.0 on Travis

9c7f216

mattbrictson merged commit 9c19abf into capistrano:master Oct 30, 2017

Fjan mentioned this pull request Nov 18, 2017

Restore ability to work with a relative path outside a within block #411

Merged

Teach upload! and download! to respect within(...) #408

Teach upload! and download! to respect within(...) #408

Uh oh!

Conversation

sj26 commented Oct 24, 2017

Uh oh!

sj26 commented Oct 25, 2017

Uh oh!

mattbrictson commented Oct 26, 2017

Uh oh!

sj26 commented Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattbrictson left a comment

Choose a reason for hiding this comment

Uh oh!

mattbrictson Oct 29, 2017

Choose a reason for hiding this comment

Uh oh!

sj26 Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sj26 commented Oct 30, 2017

Uh oh!

mattbrictson commented Oct 30, 2017

Uh oh!

sj26 commented Oct 31, 2017

Uh oh!

mattbrictson commented Nov 1, 2017

Uh oh!

mattbrictson commented Nov 3, 2017

Uh oh!

sj26 commented Nov 8, 2017

Uh oh!

Fjan commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattbrictson commented Nov 15, 2017

Uh oh!

Fjan commented Nov 16, 2017

Uh oh!

Fjan commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sj26 commented Oct 26, 2017 •

edited

Loading

sj26 Oct 30, 2017 •

edited

Loading

Fjan commented Nov 15, 2017 •

edited

Loading

Fjan commented Nov 18, 2017 •

edited

Loading