-
Notifications
You must be signed in to change notification settings - Fork 5
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
capture subprocess failures from ansible_runner #78
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's really how the API works, OK; but reporting errors on stdout
is ... disappointing.
if r.rc != 0: | ||
# print(r.stderr.read()) | ||
return "error", ErrorOutput( | ||
f"Unable to gather facts: ({r.rc}) {r.stdout.read()}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really comes on stdout
rather than stderr
?? 🫤 If you say so. The documentation claims there are both stderr
and stdout
properties ... so is the error really be reported on stdout
? That's ... ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set this originally to r.sdterr.read()
and it was empty and I was sad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting the impression that this is a crappy library lightly wrapped around subprocess
, and I should maybe just rewrite this to use subprocess
directly instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, except for the comments left by the other reviewers.
Changes introduced with this PR
The
ansible_runner
function can have an error condition that does not create an exception. This change adds a check for the return code of the subprocess and returns a plugin error if this is not 0.By contributing to this repository, I agree to the contribution guidelines.