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

put() empty remote_path arg ignores use of with cd(): #370

Closed
wants to merge 2,211 commits into from

Conversation

GaretJax
Copy link

Description

When using put() in this way:

with cd("/etc/apache2"):
    put('%s/httpd-vhosts.conf' % confs_dir, '', use_sudo=True)
    put('%s/apache2.conf' % confs_dir, '', use_sudo=True)
    sudo('mkdir logs')`

the empty string in put's second arg is supposed to resolve to the current working directory on the remote end. However, in this case it resolves to the user's home directory.

I believe the lines where this is determined are 397-400 in operations.py:

# Expand tildes (assumption: default remote cwd is user $HOME)
home = ftp.normalize('.')

# Empty remote path implies cwd
remote_path = remote_path or home

Originally submitted by Matt DeBoard (mdeboard) on 2011-06-29 at 03:32pm EDT

@ghost ghost assigned bitprophet Aug 19, 2011
@bitprophet
Copy link
Member Author

Jeff Forcier (bitprophet) posted:


Specifically, what needs to happen is the application of cd (in put) and lcd (in get, assuming it has the same problem) must occur before the substitution of $HOME for empty arguments. Hopefully doing this swap will not cause other problems; double check.

(also: updated the ticket subject to be more accurate.)


on 2011-06-29 at 03:53pm EDT

@kbd
Copy link

kbd commented Sep 12, 2011

Btw, pretty sure lcd does have the same issue, last time I checked.

Re fabric#21.

Next up:
* Make execute() actually implement what is stated in the docs
* Quite possibly update that to wrap everything in Task, then move
  interesting bits into Task.run or w/e
* Probably add more examples to the docs?
New tests not needed at this time, host/role/exclude all tested
against get_hosts() already.

Re fabric#21
Specifically:

* Replace side-effect-using `interpret_host_string` with `to_dict`/`from_dict` (for use with `settings`)
* Cleanly set up an env dict for use when calling the actual task
* Update tests appropriately

Re fabric#21
My misunderstanding: @with_fakes does not call clear_expectations, only clear_calls. Dumb IMO. May replace with my own decorator.
CLI could also have referred to the global flags.
Otherwise mocks, and any user classes defining just __call__, blow up.
bitprophet and others added 22 commits February 2, 2012 12:32
Does not actually put in the ssh config options yet.
But passes all existing tests, so good checkpoint.
Includes backpedal on host-specific ssh_config settings
overriding env vars. Not feasible with current SSHConfig,
and was conceptually messy anyways.
Includes a bunch of tightly related changes to network.py,
and tests.
Also dropping connection/timeout stuff for now, can add that later.
Renamed second test_nested_alias() to test_nested_aliases()
@GaretJax
Copy link

GaretJax commented Feb 8, 2012

I just merged the upstream master into this branch, it should be ready to be merged into master. Is there a showstopper? If so, please report, I'm willing to correct any flaws!

@bitprophet
Copy link
Member Author

I think this does need a bit more tweaking, e.g. I started writing a test for this, and noticed that a call of with settings(cwd='subdirectory'): put('whatever.txt') was trying to upload to subdirectory/subdirectory instead of subdirectory/whatever.txt -- this is due to the existing way we deal with cwd (the abspath check shortly after the line you modified.)

In other words, my fears were right, it's not as simple as just adding the extra checks to cwd.

So there definitely need to be some tweaks somewhere, and we should be careful to check a lot of possible combinations:

  • env.cwd (which is what's set by with cd()): empty string, relative path, absolute path
  • env.lcwd: ditto
  • remote_path kwarg: empty, relative path, absolute path, contains ~, doesn't contain ~
  • local_path kwarg: ditto

Ideally we want to test all potential combos there, but for now I think we can get away with just adding a handful of the more common ones.

It should be possible to expand the test suite (tests/test_operations.py, section about put), though re: the remote cwd stuff, we'd need to make sure that the test server's mkdir functionality works, and figure out how best to call it from within the tests. Forget if existing tests use it at this point.

If you think tackling all this is feasible, by all means give it a shot, otherwise I'll try sometime.

@kuba
Copy link

kuba commented Mar 27, 2012

It seems like put+cd+~ combo doesn't work as well:

$ fab --version
Fabric 1.4.0
ssh (library) 1.7.13
$ cat fabfile.py
from fabric.api import *

def buggy_put():
    with cd("~"):
        put('foo-file', 'bar-dir')
$ ls -l foo-file
-rw-rw-r-- 1 jakub jakub 0 2012-03-27 23:11 foo-file
$ ssh host ls -ld bar-dir
drwx------ 2 jakub jakub 4096 2012-03-27 23:17 bar-dir
$ fab buggy_put         
No hosts found. Please specify (single) host string for connection: host
[host] put: foo-file -> ~/bar-dir

Fatal error: put() encountered an exception while uploading 'foo-file'

Underlying exception:
    No such file

Aborting.
Disconnecting from host... done.

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

Successfully merging this pull request may close these issues.

None yet