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

Revision history for this message
Francesco Banconi (frankban) wrote :

*** Submitted:

Fix ServiceDeploy panic when called via the API.

Fix the panic that happens when calling ServiceDeploy via the juju-core
API.

Add a global charm.CacheDir variable, in order to avoid the call to
config.JujuHomePath inside the CharmStore.Get method.

charm.CacheDir is originally empty, and is then set by the
machineAgent.Run
method.

Pre-implementation call with Roger.

R=rog, TheMue
CC=
https://codereview.appspot.com/8566043

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")
On 2013/04/09 16:22:59, rog wrote:
> 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.

Done.

https://codereview.appspot.com/8566043/diff/7001/cmd/juju/main.go#newcode31
cmd/juju/main.go:31: os.Exit(1)
On 2013/04/09 16:22:59, rog wrote:
> i know it was like this before, but i think this should probably be
os.Exit(2).

Done.

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.
On 2013/04/09 16:22:59, rog wrote:
> // 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.

> ?

Done.

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

« Back to merge proposal