Code review comment for lp:~thumper/juju-core/juju-run-cli

On 2014/01/10 03:40:49, thumper wrote:
> Please take a look.

> https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go
> File cmd/juju/run.go (right):

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode33
> cmd/juju/run.go:33: Run the commands on the specified targets.
> On 2014/01/10 02:13:53, axw wrote:
> > This just occurred to me: should we be supporting patterns, like in
juju
> status?
> > e.g. juju run --service='nova-*' <commands>
> >
> > Just a thought.

> I do think that this is a good idea, but not for the first release :-)

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode128
> cmd/juju/run.go:128: var results = make([]interface{}, 0)
> On 2014/01/10 02:13:53, axw wrote:
> > Why are you initialising this with 0 length, and then appending? Why
not
> > initialise with len(runResults) and assign elements?

> Hmm good point. Since we know the size, I'll do that.

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run.go#newcode139
> cmd/juju/run.go:139: values["Stdout"] = string(result.Stdout)
> On 2014/01/10 02:13:53, axw wrote:
> > You're assuming that stdout/stderr are UTF-8 here, which they may
not be. At
> the
> > least, can we please use utf8.Valid before converting them to
strings?

> Now base64 encoding non-utf8 byte slice results.

> https://codereview.appspot.com/50310043/diff/1/cmd/juju/run_test.go
> File cmd/juju/run_test.go (right):

https://codereview.appspot.com/50310043/diff/1/cmd/juju/run_test.go#newcode210
> cmd/juju/run_test.go:210: },
> On 2014/01/10 02:13:53, axw wrote:
> > In light of my comments in run.go, I think a test for binary
stdout/stderr is
> in
> > order.

> Added.

Thanks, LGTM

https://codereview.appspot.com/50310043/