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

'abort_on_prompts' overrides warn_only = True #762

Open
pkittenis opened this issue Oct 22, 2012 · 12 comments
Open

'abort_on_prompts' overrides warn_only = True #762

pkittenis opened this issue Oct 22, 2012 · 12 comments

Comments

@pkittenis
Copy link

Trying to use both abort_on_prompts and warn_only = True makes abort_on_prompts override warn_only and fabric still throws a SystemExit

In [6]: from fabric.api import sudo, execute, cd, env, run, local

In [7]: with settings(warn_only = True):
    result = local("ls -ltrh")
   ...:     
[localhost] local: ls -ltrh
total 188K
In [8]: with settings(warn_only = True):
    result = local("ls -ltrh /tmp/tartratrat")
   ...:     
[localhost] local: ls -ltrh /tmp/tartratrat
ls: cannot access /tmp/tartratrat: No such file or directory

Warning: local() encountered an error (return code 2) while executing 'ls -ltrh /tmp/tartratrat'


In [9]: env['abort_on_prompts'] = True

In [10]: with settings(warn_only = True):
    result = local("ls -ltrh /tmp/tartratrat")
   ....:     
[localhost] local: ls -ltrh /tmp/tartratrat
ls: cannot access /tmp/tartratrat: No such file or directory

Warning: local() encountered an error (return code 2) while executing 'ls -ltrh /tmp/tartratrat'


In [11]: with settings(warn_only = True):
    result = run("ls -ltrh /tmp/tartratrat")
   ....:     

Fatal error: Needed to prompt for the target host connection string, but abort-on-prompts was set to True

Aborting.
An exception has occurred, use %tb to see the full traceback.

SystemExit: 1
To exit: use 'exit', 'quit', or Ctrl-D.
@bitprophet
Copy link
Member

(Protip, use the "Github Flavored Markdown" link above text fields, it will show you how to format things correctly. Fixed your description for you :))

I would argue that in most cases when users toggle abort_on_prompts, they are probably expecting the current behavior (abort) and not a warn-and-continue. warn_only=True typically applies to what to do when the remote program has a failure, vs Fabric itself aborting.

However I acknowledge that this situation where both are True, is not well defined/documented. Additionally, most other uses of abort() in the codebase have been replaced with calls to error() which is what handles the "abort if warn_only=False" logic.

I'll make the call for now that consistency trumps the possibility of a user trying to abort-on-prompts and being thwarted by an unexpected warn_only=True, and will make the change you're requesting. If other folks come in complaining I will put you out in front though ;)

@bitprophet
Copy link
Member

On reflection, I'm really torn on this; I can see the change screwing things up for users who, as mentioned, truly do want Fabric to blow up on unexpected prompts. Hidden errors can be incredibly frustrating to track down.

@pkittenis -- what real world situation are you running into where this is a problem for you? Would you be able to use try/except SystemExit to work around it?

If you're hard blocked by this, what might make more sense is to add some new config option (with some dumb name like warn_trumps_abort perhaps) so people in your use case can control behavior, without causing an unexpected change for current users.

@pkittenis
Copy link
Author

The issue is that the behaviour of abort_on_prompts causes fabric to throw a SystemExit when it falls back to No hosts found. Please specify (single) host string for connection: when a command has an error.

When using fabric as an API, that is just wrong IMO. The user of the API has already set warn_only to avoid a SystemExit on the execute and instead be able to handle return codes within code. That's great.

Then the user sets abort_on_prompts and instead of getting the error code for his command when it fails, they get a SystemExit because of the fallback to enter a host and the use of abort_on_prompts. Can handle the exception, sure, but in that case you can't see the returncode of the command that failed anymore.

Can work-around for now by resetting abort_on_prompts back to False but that needs to be done on every command that may or may not fail,

I suppose what would be best would be a flag to completely disable built in fabric prompts that are meant to be used on the command line, particurarly the No hosts found.. one, when using fabric as an API. Then abort_on_prompts would not cause a SystemExit since there is no prompt to cause it.

@amontalenti
Copy link

+1 -- I am running into this very issue right now. My concrete use case is that I am using Fabric in order to do a check on recently-provisioned cloud nodes and confirm whether those nodes have SSH access with the appropriate public key. As a result, I need to actually catch the abort_on_prompts exception. For now I'm just going to explicitly catch SystemExit, but this feels very wrong indeed.

@nicholaskuechler
Copy link

+1 -- I would also like to catch abort_on_prompts exceptions.

@tebeka
Copy link

tebeka commented May 1, 2014

👍 I'm in the same situation where I don't want SystemExit to be raised - it kills my celery workers.

@bitprophet
Copy link
Member

At this stage I feel this level of change fits better in version 2.0 which is being designed with the API use case first & the CLI use case as a secondary/wrapper use case. Changing the behavior in 1.x will be too surprising/confusing/liable to add bugs.

Tagging as 2.x and leaving open so I can hopefully revisit & make sure I consider it when writing that part of the API.

@bitprophet bitprophet added 2.x and removed 2.x labels Aug 4, 2014
@4hr1m4n
Copy link

4hr1m4n commented Mar 5, 2015

+1 -- when both abort_on_prompts and warn_only are True, warn_only should prevail.
Another thread on this issue, by @reeesga: #521

@ybv
Copy link

ybv commented Jun 19, 2015

+1

@balajijegan
Copy link

+1 -- when both abort_on_prompts and warn_only are True, warn_only should prevail. A newer setting to do this is fine as well

@ocervell
Copy link

Any update on this ?
I catched SystemExit but it just makes my output ugly ...

See by yourself:

webapps2adm001.qa.aws.company.com : Connection as root SUCCESS.
webapps2001.qa.aws.company.com : Connection as root SUCCESS.

Fatal error: Needed to prompt for a connection or sudo password (host: webapps3001.ia.aws.company.com), but input would be ambiguous in parallel mode

Aborting.
webapps3001.ia.aws.company.com : Connection as root FAILED. SystemExit: 1
webapps3adm001.ia.aws.company.com : Connection as root SUCCESS.

I just want to keep the line after Aborting. I don't want to see the error messages.

@bitprophet bitprophet modified the milestones: 2.0, Missed 2.0, 1.15 (minor 1.x feature adds) Jul 10, 2018
@bitprophet bitprophet removed this from the 1.15 (minor 1.x feature adds) milestone Nov 27, 2018
@mitcharf
Copy link

I'm also running into this issue and would love to see the current behavior changed.

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

No branches or pull requests

10 participants