-
Notifications
You must be signed in to change notification settings - Fork 231
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
Seperate stdout and stderr #615
Conversation
@@ -244,7 +244,7 @@ func TestZeusBoots(t *testing.T) { | |||
|
|||
cexit := make(chan int, 1) | |||
go func() { | |||
cexit <- zeusclient.Run([]string{"cmd"}, hangingReader{readCloser}, cmdWriter) | |||
cexit <- zeusclient.Run([]string{"cmd"}, hangingReader{readCloser}, cmdWriter, cmdWriter) |
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.
maybe create 2 seperate writers here... and actually test that it works?
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.
Done
@@ -144,12 +144,13 @@ def command(identifier, sock) | |||
|
|||
plan.after_fork | |||
client_terminal = local.recv_io | |||
client_terminal_stderr = local.recv_io |
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.
client_terminal should be client_stdout... and than we have sane naming.
break | ||
} | ||
} | ||
}() |
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.
There is a lot of duplication here, should create seperate functions of doing setup and read and then just pass in the different streams...
if os.Getenv("RAILS_ENV") != "" { | ||
println("Warning: Specifying a Rails environment via RAILS_ENV has no effect for commands run with zeus.") | ||
println("As a safety precaution to protect you from nuking your development database,") | ||
println("Zeus will now cowardly refuse to proceed. Please unset RAILS_ENV and try again.") | ||
return 1 | ||
} | ||
|
||
// if stdout is a terminal, assume that stderr is a terminal as well |
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.
Not sure if this is a save assumption, probably better to check either and do the right thing.
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.
Done
@@ -178,6 +246,7 @@ func Run(args []string, input io.Reader, output *os.File) int { | |||
}() | |||
|
|||
<-eof | |||
<-eofStderr |
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.
Should use a Waitgroup https://golang.org/pkg/sync/#WaitGroup here
I always felt like there was some legitimate reason that I didn't do this originally, but now that I think about it, it was probably just confusion or laziness. Looks good so far. |
d60cb63
to
8410041
Compare
Ok the code has been cleaned up, this now needs some more testing as the coverage is not ideal. I tried to localise the cases where things need to be reset as well via defer so maybe this resolves some of the issue where zeus seems to messup the shell as well. |
It is often helpful to have seperate streams for stdout and stderr especially when grepping stdout I still want to see errors on stderr. - adding test for stderr logging - Unneeded restore termial state, use defer where possible - Use waitgroup to await end of io interaction
8410041
to
0916d0f
Compare
It is often helpful to have seperate streams for stdout and stderr especially
when grepping stdout I still want to see errors on stderr.