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

Allows the creation of custom prompts and responses to allow custom prompts for contextmanagers. #741

Merged
merged 4 commits into from Dec 17, 2013

Conversation

@owenn
Copy link

@owenn owenn commented Sep 27, 2012

Allows the creation of custom prompts and responses. I wrote it to allow custom contextmanagers as in the following example:

from fabric.api import env, run
import contextlib

@contextlib.contextmanager
def asu(user, asu_number):
    '""custom ASU wrapper around sudo"""

    save_shell = env.shell

    save_current_prompts = env.prompt_responses
    env.prompt_responses = { "Enter ASU #:" : asu_number, "Password:" : "testpassword" }

    env.shell = '$HOME/asu  %s -c' % user
    yield
    env.shell = save_shell
    env.prompt_responses = save_current_prompts

And in the fab file:

with asu("root",asu_number):
      run("touch /var/tmp/test.touch")
setup.py Outdated
@@ -37,7 +37,7 @@
url='http://fabfile.org',
packages=find_packages(),
test_suite='nose.collector',
tests_require=['nose', 'fudge<1.0'],
tests_require=['nose', 'fudge<1.1'],

This comment has been minimized.

@bitprophet

bitprophet Oct 4, 2012
Member

Why the fudge bump? IIRC I ran into test problems with our current suite on Fudge 1.0.x (i.e. an API change in 1.0 broke us). I have not verified that in a while though, just that 0.9.x works fine.

@@ -302,6 +302,7 @@ def _rc_path():
'sudo_prefix': "sudo -S -p '%(sudo_prompt)s' ",
'sudo_prompt': 'sudo password:',
'sudo_user': None,
'request_prompts' : {},

This comment has been minimized.

@bitprophet

bitprophet Oct 4, 2012
Member

Not a huge fan of this variable name -- how about prompt_responses?

)
eq_(_get_prompt_response(list("this is a prompt for prompt1"),ad['request_prompts']),("prompt1","response1"))
eq_(_get_prompt_response(list("this is a prompt for prompt2"),ad['request_prompts']),("prompt2","response2"))
eq_(_get_prompt_response(list("this is a prompt for promptx:"),ad['request_prompts']),None)

This comment has been minimized.

@bitprophet

bitprophet Oct 4, 2012
Member

PEP8 is weeping at your lack of spaces after commas here :D Same problem in a few other spots too. Please fix ;)

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Oct 4, 2012

Currently +1 on the overall idea, I added some nitpicks about the implementation though.

I need to test this on my end to make sure the core sudo prompt behavior still functions correctly, but the changeset looks like that shouldn't be a problem.

Also noting for posterity that this is related to, but might not 100% duplicate, #177.

@owenn
Copy link
Author

@owenn owenn commented Oct 5, 2012

Thanks for the comments do you want me to fix the issues you
mentioned ? I had a look a the pexpect library and I thought there was
potential there. The general thought was that I could expand the stuff
I just wrote to include triggers, if a certain predefined pattern is
matched control would then be passed to a pexpect section which once
executed would pass control back to Fabric, though 177 mentions a
mismatch between file descriptors and whatever Paramiko is offering,
so no idea how easy that is. Additionally fabric is currently pretty
simple to understand and I have no idea of what the tolerance is to
complicate things to that level, hence this implementation which I
think addresses most cases anyway, and integrating pexpect would also
take a bit of time.

And thanks for the pep8 comment, I am just starting trying to follow
pep8 as I am new to python, so appreciate anything you have to say.

  • Nigel

On 5/10/2012, at 10:40 AM, Jeff Forcier wrote:

Currently +1 on the overall idea, I added some nitpicks about the
implementation though.

I need to test this on my end to make sure the core sudo prompt
behavior still functions correctly, but the changeset looks like
that shouldn't be a problem.

Also noting for posterity that this is related to, but might not
100% duplicate, #177.


Reply to this email directly or view it on GitHub.

@owenn
Copy link
Author

@owenn owenn commented Oct 5, 2012

I have hopefully addressed everything you mentioned.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Oct 28, 2012

I'm looking at this ASAP, apologies for the delay! Re: pexpect, definitely punting on using that for now; Fab 2 may take another stab at integrating it.

for request_prompt in prompt_responses.keys():
if _endswith(capture, request_prompt):
return (request_prompt, prompt_responses[request_prompt])
return None

This comment has been minimized.

@bitprophet

bitprophet Oct 28, 2012
Member

Python implicitly returns None by default, so this line can go.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Oct 28, 2012

I think this works for now (see one additional nitpick above.) Github claims it needs to be rebased/merged onto latest master, though -- you might want to check on that.

If you could handle some quick doc additions too, I can just hit merge and go on:

  • Add a changelog entry, following existing style, including crediting yourself if you want;
  • Add an entry in docs/usage/env.rst for the prompt_responses env var, again following existing entries in that file (and putting it in alphabetical order).

Thanks for your patience!

@MajorTal
Copy link

@MajorTal MajorTal commented Sep 10, 2013

What are the plans for this pull request?

@davidhalter
Copy link

@davidhalter davidhalter commented Oct 15, 2013

+1

If the above points are the only reasons why this never got merged, I would like to take this PR and fix the problems. If @owenn doesn't mind.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Oct 23, 2013

@davidhalter Feel free, since @owenn hasn't responded yet. If you do please credit both of you in the changelog :) thanks!

I'm trying to be on a tighter ticket feedback loop these days (max of a week before at least checking in, if not merging) so I'll do my best not to leave it sitting around for too long afterwards.

@davidhalter
Copy link

@davidhalter davidhalter commented Nov 12, 2013

Sorry for being quiet for so long @bitprophet. I just transitioned out of a country.

I tried to implement all of #1021 and hope I haven't forgotten anything.

@bitprophet bitprophet merged commit 310e479 into fabric:master Dec 17, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.