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

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

Please take a look.

https://codereview.appspot.com/41660044/diff/1/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/41660044/diff/1/cmd/cmd.go#newcode20
cmd/cmd.go:20: type rcPassthroughError struct {
On 2013/12/13 04:17:33, wallyworld wrote:
> Why not use a type alias?

I think because I want a real struct here. A type alias to int is just
weird.

https://codereview.appspot.com/41660044/diff/1/cmd/cmd.go#newcode33
cmd/cmd.go:33: func NewRcPassthroughError(code int) error {
On 2013/12/13 04:17:33, wallyworld wrote:
> Need doc here and/or the struct definition to say what it is for

Done.

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/main.go
File cmd/jujud/main.go (right):

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/main.go#newcode118
cmd/jujud/main.go:118: code = cmd.Main(&RunCommand{},
cmd.DefaultContext(), args[1:])
On 2013/12/13 04:17:33, wallyworld wrote:
> Should we follow the pattern used for jujuc and jujud and have a
jujurunMain
> method?

We could, but it would just be one line... so I chose not to.

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/run.go
File cmd/jujud/run.go (right):

https://codereview.appspot.com/41660044/diff/1/cmd/jujud/run.go#newcode67
cmd/jujud/run.go:67: c.unit = names.UnitTag(c.unit)
On 2013/12/13 04:17:33, wallyworld wrote:
> can we have a comment here explaining what is being done ie unit can
be either a
> name or tag?

Done.

https://codereview.appspot.com/41660044/diff/1/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/41660044/diff/1/environs/cloudinit/cloudinit.go#newcode242
environs/cloudinit/cloudinit.go:242: fmt.Sprintf("ln -s
%s/tools/%s/jujud /usr/local/bin/juju-run", cfg.DataDir, machineTag),
On 2013/12/13 04:17:33, wallyworld wrote:
> please use cfg.jujuTools()

No, but comment added to explain why.

https://codereview.appspot.com/41660044/

« Back to merge proposal