update mstate charm usage

update mstate charm usage

Service/Unit now have charm-related methods that match state

https://code.launchpad.net/~fwereade/juju-core/mstate-service-unit-charm/+merge/124672

(do not edit description out of merge proposal)

[fwereade, 2012-09-17 13:10:47]

Please take a look.

[niemeyer, 2012-09-17 14:33:48]

https://codereview.appspot.com/6506124/diff/1/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/6506124/diff/1/charm/url.go#newcode181
charm/url.go:181: if u == nil {
It'd be nice to have a test for this change. Roger just committed a couple of
tests to juju-core/version/version_test.go that can be copied and tweaked to
check for the nil case as well.

https://codereview.appspot.com/6506124/diff/1/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6506124/diff/1/mstate/service.go#newcode102
mstate/service.go:102: // Charm returns the service's charm, and whether units
should upgrade to that
s/,//?

https://codereview.appspot.com/6506124/diff/1/mstate/service.go#newcode104
mstate/service.go:104: func (s *Service) Charm() (*Charm, bool, error) {
We can make good use of the result names in this case. It's not clear what that
bool means otherwise.

https://codereview.appspot.com/6506124/diff/1/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6506124/diff/1/mstate/unit.go#newcode234
mstate/unit.go:234: Assert: D{{"life", Alive}},
I wonder if we really want that assertion in this case. Imagine we're in the
process of upgrading the machine, and then we've just finished it and want to
track the new version we've used. If someone set the life to Dying meanwhile, as
a way to say "Hey, can you please terminate at the next opportunity", this
assert would blow up that otherwise ok change. With that in mind, I suspect
txn.DocExists would be enough here.

[fwereade, 2012-09-17 17:14:48]

Please take a look.

https://codereview.appspot.com/6506124/diff/1/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/6506124/diff/1/charm/url.go#newcode181
charm/url.go:181: if u == nil {
On 2012/09/17 14:33:48, niemeyer wrote:
> It'd be nice to have a test for this change. Roger just committed a couple of
> tests to juju-core/version/version_test.go that can be copied and tweaked to
> check for the nil case as well.

Done.

https://codereview.appspot.com/6506124/diff/1/mstate/service.go
File mstate/service.go (right):

https://codereview.appspot.com/6506124/diff/1/mstate/service.go#newcode102
mstate/service.go:102: // Charm returns the service's charm, and whether units
should upgrade to that
On 2012/09/17 14:33:48, niemeyer wrote:
> s/,//?

Done.

https://codereview.appspot.com/6506124/diff/1/mstate/service.go#newcode104
mstate/service.go:104: func (s *Service) Charm() (*Charm, bool, error) {
On 2012/09/17 14:33:48, niemeyer wrote:
> We can make good use of the result names in this case. It's not clear what
that
> bool means otherwise.

Done.

https://codereview.appspot.com/6506124/diff/1/mstate/unit.go
File mstate/unit.go (right):

https://codereview.appspot.com/6506124/diff/1/mstate/unit.go#newcode234
mstate/unit.go:234: Assert: D{{"life", Alive}},
On 2012/09/17 14:33:48, niemeyer wrote:
> I wonder if we really want that assertion in this case. Imagine we're in the
> process of upgrading the machine, and then we've just finished it and want to
> track the new version we've used. If someone set the life to Dying meanwhile,
as
> a way to say "Hey, can you please terminate at the next opportunity", this
> assert would blow up that otherwise ok change. With that in mind, I suspect
> txn.DocExists would be enough here.

Done.

[niemeyer, 2012-09-17 17:24:04]

LGTM, thanks!

[niemeyer, 2012-09-17 17:24:04]

LGTM, thanks!

[fwereade, 2012-09-17 17:26:13]

*** Submitted:

update mstate charm usage

Service/Unit now have charm-related methods that match state

R=niemeyer
CC=
https://codereview.appspot.com/6506124