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

contrib.files.append([...], use_sudo_=True) does not work with 'cd()' ctx manager #703

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@mikek

mikek commented Aug 21, 2012

with settings(user=some_user), cd('/home/some_user'):
    append('.ssh/authorized_keys', key, partial=True, use_sudo=use_sudo)

This code silently duplicates existing key in a checked file. The full resulting command is:

sudo -S -p 'sudo password:'  cd /home/some_user && egrep "^ssh\-rsa\ AAAAB3Nza[...]jN\+v7w\=\=" ".ssh/authorized_keys"

(as returned by _shell_wrap() call) The problem is: normally, there is no standalone 'cd' command in UNIX, as it is a shell built-in. And ommiting context manager #with settings(hide('everything'), warn_only=True): at the end of contains() reveals that.

Quick test to use shell=True in the final call inside contains() fixes that, but possibly has other side-effects.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 16, 2012

Yes, adding shell=False to contains and sed were done to remove one level of quoting which was causing issues in various situations, but I can see how that would sink the way cd behaves.

I don't see a good way to have our cake and eat it too, but am open to suggestions. Best I can think of is to add this as a warning in the docstrings in contrib, and an option to these affected methods to allow overriding the shell=False.

@mikek

This comment has been minimized.

mikek commented Aug 19, 2012

It seems running commands this way cd /home/foo && sudo -S -p 'sudo password:' egrep [...] is not what we want.
So here is the quick "workaround" you've suggested. I am not sure where the general warning should go and if the 1.4.4 is the right version to mention in versionchanged. Can you look at this branch and tell if it is OK to send this as a pull request?

@bitprophet

This comment has been minimized.

Member

bitprophet commented Aug 20, 2012

That branch looks good, besides two nitpicks:

  • 1.5's the right version to use -- this is an API change (even though it's in the service of fixing something bug-ish).
  • Please add an entry at the top of docs/changelog.rst following the style of existing entries, e.g. "Add a shell kwarg to many methods in ~fabric.contrib.files to help avoid conflicts with cd and similar`".

Re: the PR, I'd recommend using hub pull-request to add the PR to this ticket so we avoid having two tickets.

Thanks!

@mikek

This comment has been minimized.

mikek commented Nov 25, 2012

Do you have any objections on this (the 1.5.x is already out now)?

bitprophet added a commit that referenced this pull request Jan 25, 2013

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jan 25, 2013

All set now; updated versionadded bits to 1.6 since 1.5 came out a little while ago. Thanks again, & apologies for the delay!

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