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

Revision history for this message
Tim Penhey (thumper) wrote :

Please take a look.

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()
On 2014/01/10 01:23:58, axw wrote:
> command.Stdin = strings.NewReader(params.Command+"\n")

Done.

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

OK.

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

sure, although this is paranoia as there is no other return options.

https://codereview.appspot.com/49940045/diff/1/utils/ssh/run.go#newcode67
utils/ssh/run.go:67: status :=
ee.ProcessState.Sys().(syscall.WaitStatus)
On 2014/01/10 01:23:58, axw wrote:
> 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.

Perhaps, but the CLI isn't doing this, the apiserver is.

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

« Back to merge proposal