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

make combinedOutput actually output both stdout and stderr #881

Merged
merged 1 commit into from Jul 31, 2017

Conversation

Projects
None yet
3 participants
@erizocosmico
Copy link
Contributor

erizocosmico commented Jul 22, 2017

What does this do / why do we need it?

When the command exits with status 0, combinedOutput should return the output of both stderr and stdout combined. Previously, it only returned stdout contents.

What should your reviewer look out for in this PR?

Maybe the syncBuffer is a bit of an overkill? After all, stdlib's CombinedOutput uses a bytes.Buffer for both without any mutex.

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

I think none, just saw the TODO and started fixing this

@erizocosmico erizocosmico requested a review from sdboyer as a code owner Jul 22, 2017

@googlebot googlebot added the cla: yes label Jul 22, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 23, 2017

ahh, that's the license header checker. (need a rebase, too)

@erizocosmico erizocosmico force-pushed the erizocosmico:fix/cmd-combined-output branch 2 times, most recently from 7b470dc to 61047b0 Jul 23, 2017

@erizocosmico

This comment has been minimized.

Copy link
Contributor

erizocosmico commented Jul 23, 2017

Oopsie, fixed!

@@ -22,6 +23,7 @@ import (
type monitoredCmd struct {
cmd *exec.Cmd
timeout time.Duration
output *syncBuffer

This comment has been minimized.

@sdboyer

sdboyer Jul 26, 2017

Member

yeah, this seems like it might be overkill, per your note in the OP.

what if instead, if the user calls monitoredCmd.combinedOutput(), then we just reuse one of the activityBuffers - e.g., assign cmd.stdout to cmd.stderr? i can't think of any immediate problems with that...

@erizocosmico erizocosmico force-pushed the erizocosmico:fix/cmd-combined-output branch 2 times, most recently from 4323cd0 to 94ceb20 Jul 26, 2017

make combinedOutput actually output both stdout and stderr
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
@erizocosmico

This comment has been minimized.

Copy link
Contributor

erizocosmico commented Jul 26, 2017

Done @sdboyer!

@sdboyer
Copy link
Member

sdboyer left a comment

yes! if this works, then it LGTM.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 26, 2017

cool!

hmm, that failure's on a test that i believe we're refactoring, anyway. easiest thing is probably to hang tight for a few days while we get that sorted.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Jul 31, 2017

eh, i'm not going to leave this hanging - the failure is unrelated. in we go.

@sdboyer sdboyer merged commit b6765a5 into golang:master Jul 31, 2017

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
cla/google All necessary CLAs are signed
codeclimate 1 fixed issue
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment