Code review comment for lp:~thumper/juju-core/ssh-run-on-remote

Revision history for this message
Andrew Wilkins (axwalk) wrote :

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go
File utils/ssh/run.go (right):

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode44
utils/ssh/run.go:44: input, err := command.StdinPipe()
command.Stdin = strings.NewReader(params.Command+"\n")

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode51
utils/ssh/run.go:51: var commandDone = make(chan error)
Style nit, I think := is appropriate here.
Also, the channel doesn't need to be created until after Start returns
successfully.

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode59
utils/ssh/run.go:59: close(commandDone)
Maybe a bit paranoid, but I think it's good defensive programming to
defer the close at the top.

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode67
utils/ssh/run.go:67: status :=
ee.ProcessState.Sys().(syscall.WaitStatus)
This is not going to work on the Windows CLI, is it?

You can use ee.ProcessState.Exited(), but I don't think there's a
platform-independent way of getting the exit status. A helper may be in
order.

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run_test.go
File utils/ssh/run_test.go (right):

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run_test.go#newcode105
utils/ssh/run_test.go:105: while read line
cat /dev/stdin?

https://codereview.appspot.com/49940045/

« Back to merge proposal