Fix ServiceDeploy panic when called via the API.

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.

https://code.launchpad.net/~frankban/juju-core/bug-1166224-service-deploy-panic/+merge/157836

(do not edit description out of merge proposal)

[frankban, 2013-04-09 10:06:32]

Please take a look.

[rog, 2013-04-09 11:24:13]

not lgtm for the moment - the CharmDir is not set when the juju command is run.

please could you move the checkJujuHome function from cmd/juju/main.go into the
juju package, rename it to InitJujuHome, and add a line to set charm.CharmDir
appropriately.

https://codereview.appspot.com/8566043/diff/1/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode214
charm/repo.go:214: // Get returns the charm referenced by curl.
// CacheDir must have been set, otherwise Get will panic.

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode216
charm/repo.go:216: // MachineAgent.Run should have already set the CacheDir.
// The cache location must have been previously set.
(it might have been set directly too, or via juju.SetJujuHome)

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode218
charm/repo.go:218: panic("the charm cache directory path is empty")
s/the //

just convention.

[TheMue, 2013-04-09 11:25:39]

LGTM

[TheMue, 2013-04-09 11:30:07]

On 2013/04/09 11:24:13, rog wrote:
> not lgtm for the moment - the CharmDir is not set when the juju command is
run.
> 
> please could you move the checkJujuHome function from cmd/juju/main.go into
the
> juju package, rename it to InitJujuHome, and add a line to set charm.CharmDir
> appropriately.

Good catch, missed it.

[frankban, 2013-04-09 15:51:50]

Please take a look.

https://codereview.appspot.com/8566043/diff/1/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode214
charm/repo.go:214: // Get returns the charm referenced by curl.
On 2013/04/09 11:24:13, rog wrote:
> // CacheDir must have been set, otherwise Get will panic.

Done.

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode216
charm/repo.go:216: // MachineAgent.Run should have already set the CacheDir.
On 2013/04/09 11:24:13, rog wrote:
> // The cache location must have been previously set.
> (it might have been set directly too, or via juju.SetJujuHome)

Done.

https://codereview.appspot.com/8566043/diff/1/charm/repo.go#newcode218
charm/repo.go:218: panic("the charm cache directory path is empty")
On 2013/04/09 11:24:13, rog wrote:
> s/the //
> 
> just convention.

Done.

[rog, 2013-04-09 16:22:59]

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.

?

[frankban, 2013-04-09 16:43:23]

*** 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.