Code review comment for lp:~frankban/juju-core/bug-1166224-service-deploy-panic

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM with the below points addressed.
thanks!

https://codereview.appspot.com/8566043/diff/7001/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/8566043/diff/7001/cmd/juju/main.go#newcode30
cmd/juju/main.go:30: fmt.Fprintf(os.Stderr, "command failed:
"+err.Error()+"\n")
fmt.Fprintf(os.Stderr, "error: %s\n", err)

to be consistent with cmd.Main

in general passing a non-constant argument to Fprintf-like functions is
not a good idea regardless.

https://codereview.appspot.com/8566043/diff/7001/cmd/juju/main.go#newcode31
cmd/juju/main.go:31: os.Exit(1)
i know it was like this before, but i think this should probably be
os.Exit(2).

https://codereview.appspot.com/8566043/diff/7001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8566043/diff/7001/juju/conn.go#newcode413
juju/conn.go:413: // It also sets up the charm cache directory path in
charm.CacheDir.
// InitJujuHome initializes the charm and environs/config
// packages to use default paths based on the
// $JUJU_HOME or $HOME environment variables.
// This function should be called before calling NewConn or
// Conn.Deploy.

?

https://codereview.appspot.com/8566043/

« Back to merge proposal