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

Change to CombinedOutput to consume Stderr as well #144

Merged
merged 3 commits into from
Jun 16, 2017

Conversation

ilkelma
Copy link
Contributor

@ilkelma ilkelma commented Jun 13, 2017

First off thank you for this awesome work. It's been super easy to get the (usually) really hard stuff set up with this tool!

I did run into one issue where I thought things could be improved a bit though. One of my hooks runs a small go binary and in that binary errors are written to os.Stderr and when testing I noticed that it was failing with no feedback other than "exited with error code $code" which was not terribly helpful. I checked the code and noticed you were using Cmd.Output() which only grabs os.Stdout by default.

I've submitted this pull request changing that behavior to use CombinedOutput which consumes both stderr and stdout. Hopefully that doesn't make the output too verbose but I was easily able to discover the problem in my code upon changing it and so I thought others might benefit as well.

Thank you again!

@adnanh
Copy link
Owner

adnanh commented Jun 15, 2017

Hey, thank you for your contribution! I agree with you that we should include stderr in our logs as well, especially because we usually check logs when something is misbehaving, and they should be as verbose as possible to help us identify the real cause!

Before I can merge the PR, could you please just create it against a development branch, not the master :-)

@ilkelma ilkelma changed the base branch from master to development June 15, 2017 14:03
@ilkelma
Copy link
Contributor Author

ilkelma commented Jun 15, 2017

Ok I think I've done that but it brought along a couple other changes. I'll leave this open as is for now but I can definitely close this one and open a new one (since it's such a small change) to avoid messing up the tree or in general bringing over stuff that shouldn't be in the development branch yet.

@adnanh
Copy link
Owner

adnanh commented Jun 16, 2017

This is good. Thank you!

@adnanh adnanh merged commit c581e05 into adnanh:development Jun 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants