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

Show stderr when error occurs on command execution #39

Merged
merged 1 commit into from
Mar 4, 2017
Merged

Show stderr when error occurs on command execution #39

merged 1 commit into from
Mar 4, 2017

Conversation

isimluk
Copy link
Member

@isimluk isimluk commented Feb 23, 2017

Awesome spawn is used to run external utilities. Sometimes rare error occurs, then we through exception like this:

AwesomeSpawn::CommandResultError: psql exit code: 2

Usually there is an stderr that is contains useful details that could help admins, operators, supporters, developers and qa. Let's publish this stderr to give quick clue.

@coveralls
Copy link

coveralls commented Feb 23, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5c53830 on isimluk:publish-stderr-when-problem-occurs into 77519d4 on ManageIQ:master.

@miq-bot
Copy link
Member

miq-bot commented Feb 23, 2017

Checked commit https://github.com/isimluk/awesome_spawn/commit/5c53830b9613f0f6edba9cb02a569deef1bca428 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🏆

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@gtanzillo
Copy link
Member

@jrafanie Please review

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, we might have suppressed the error in case there were passwords or something else logged or printed from standard error. It's probably better to just fix those rare occasions where we need to sanitize the output.

Do we need to truncate the standard error output in case it's really long? If needed, we can do a followup PR.

@jrafanie jrafanie merged commit 6fa0e9b into ManageIQ:master Mar 4, 2017
@isimluk
Copy link
Member Author

isimluk commented Mar 6, 2017

Thanks!!

Note that if the password is written to stderr, it is not our bug. The problem would be in the tool we run.

@isimluk isimluk deleted the publish-stderr-when-problem-occurs branch March 6, 2017 12:08
@Fryguy
Copy link
Member

Fryguy commented Mar 6, 2017

I'm less concerned about passwords specifically than I am about other information, such as IP addresses on STDERR.

@jrafanie
Copy link
Member

jrafanie commented Mar 7, 2017

Yeah, it's really anything containing sensitive information that you may not want to capture from STDERR. But, it feels like we shouldn't cripple our ability to diagnose problems by not capturing anything so I'd rather we try to have clients sanitize command STDERR data when they consume this.

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.

6 participants