Add UpgradeCharm to the API

Add UpgradeCharm to the API

This branch adds upgrade charm functionality to the API (branch 3/3 in the process).

https://code.launchpad.net/~makyo/juju-core/upgradecharm3-1171548/+merge/167331

(do not edit description out of merge proposal)

[matthew.scott, 2013-06-12 18:54:50]

Please take a look.

[fwereade, 2013-06-13 08:01:08]

Code looks great, but a couple of tests need to be rather more unforgiving ;)

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go
File state/apiserver/client_test.go (right):

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go...
state/apiserver/client_test.go:417: mem4g := constraints.MustParse("mem=4G")
Drop the constraints, they're just a distraction here.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go...
state/apiserver/client_test.go:425: c.Assert(err, IsNil)
AFAICT this is just calling SetCharm with the existing charm in a slightly
obfuscated way. We really ought to actually change the charm, and check that the
change stuck.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go...
state/apiserver/client_test.go:440: c.Assert(err, IsNil)
Here it's fine to use the same charm, but you should still test that the force
flag was written.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go...
state/apiserver/client_test.go:456: s.setUpScenario(c)
do we need setUpScenario here? I can accept it's useful for permTest but in
general it's an awfully big fixture... better, I think, to just add a wordpress
service directly. Can't we just do:

s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))

?

[matthew.scott, 2013-06-13 16:44:44]

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go
File state/apiserver/client_test.go (right):

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go...
state/apiserver/client_test.go:417: mem4g := constraints.MustParse("mem=4G")
On 2013/06/13 08:01:08, fwereade wrote:
> Drop the constraints, they're just a distraction here.

Done.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go...
state/apiserver/client_test.go:425: c.Assert(err, IsNil)
On 2013/06/13 08:01:08, fwereade wrote:
> AFAICT this is just calling SetCharm with the existing charm in a slightly
> obfuscated way. We really ought to actually change the charm, and check that
the
> change stuck.

Done.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go...
state/apiserver/client_test.go:440: c.Assert(err, IsNil)
On 2013/06/13 08:01:08, fwereade wrote:
> Here it's fine to use the same charm, but you should still test that the force
> flag was written.

Done.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go...
state/apiserver/client_test.go:456: s.setUpScenario(c)
On 2013/06/13 08:01:08, fwereade wrote:
> do we need setUpScenario here? I can accept it's useful for permTest but in
> general it's an awfully big fixture... better, I think, to just add a
wordpress
> service directly. Can't we just do:
> 
> s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))
> 
> ?

Ah, much cleaner.  Done.

[matthew.scott, 2013-06-13 16:46:06]

Please take a look.

[fwereade, 2013-06-14 15:20:56]

LGTM, thanks!

[mue, 2013-06-14 15:28:46]

LGTM

[matthew.scott, 2013-06-14 16:20:15]

*** Submitted:

Add UpgradeCharm to the API

This branch adds upgrade charm functionality to the API (branch 3/3 in the
process).

R=fwereade, mue
CC=
https://codereview.appspot.com/10237043