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

Revision history for this message
Ian Booth (wallyworld) wrote :

LGTM with some tweaks and a suggestion

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 {
Why not use a type alias?

https://codereview.appspot.com/41660044/diff/1/cmd/cmd.go#newcode33
cmd/cmd.go:33: func NewRcPassthroughError(code int) error {
Need doc here and/or the struct definition to say what it is for

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:])
Should we follow the pattern used for jujuc and jujud and have a
jujurunMain method?

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)
can we have a comment here explaining what is being done ie unit can be
either a name or tag?

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),
please use cfg.jujuTools()

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

« Back to merge proposal