state: add new peer relations on upgrade

state: add new peer relations on upgrade

When setiing a serivce's charm on upgrade, new
peer relations are now added as part of the same
transaction.

Slight refactoring of AddService is done as well,
extracting common code now used by both AS() and
service.SetCharm.

Refactored testing Add*Charm() calls to use more
generic code. In a follow-up it'll be improved
further.

https://code.launchpad.net/~dimitern/juju-core/017-upgrade-charm-add-new-peer-relations/+merge/155562

(do not edit description out of merge proposal)

[dimitern, 2013-03-26 17:24:39]

Please take a look.

[rog, 2013-03-26 17:41:57]

as discussed online, please could you make sure the revised tests in state fix
the issue you were seeing in cmd/juju. thanks!

[dimitern, 2013-03-27 09:31:38]

On 2013/03/26 17:41:57, rog wrote:
> as discussed online, please could you make sure the revised tests in state fix
> the issue you were seeing in cmd/juju. thanks!

I can confirm the issue is reproduced and caught with the reverted code and
improved tests.

[dimitern, 2013-03-27 09:32:21]

Please take a look.

[fwereade, 2013-03-27 09:49:47]

logic looks good but I'd prefer to rework the tests a bit, ping me to talk in a
mo?

https://codereview.appspot.com/8034043/diff/5001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/8034043/diff/5001/state/service.go#newcode311
state/service.go:311: func (s *Service) peerRelationsDifference(otherMeta
*charm.Meta) map[string]charm.Relation {
consider:

s/peerRelationsDifference/newPeerRelations/
s/otherMeta/newMeta/

...and similarly through the method. YMMV though.

https://codereview.appspot.com/8034043/diff/5001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/8034043/diff/5001/state/service_test.go#newcod...
state/service_test.go:297: c.Assert(ch.Meta().Peers, HasLen, 2)
I think the tests down to here are redundant -- the SetCharm tests do already
test that we set the correct charm on the service, and all the
Service.Endpoint() stuff is tested pretty comprehensively elsewhere.

https://codereview.appspot.com/8034043/diff/5001/state/service_test.go#newcod...
state/service_test.go:298: c.Assert(state.ServiceRelationCount(s.mysql), Equals,
2)
I would much prefer a black-box test that demonstrated correct behaviour on
Destroy.

https://codereview.appspot.com/8034043/diff/5001/state/service_test.go#newcod...
state/service_test.go:302: ep, err = rels[0].Endpoint("mysql")
I don't think it's appropriate to assume an ordering for the result of
Relations() here.

[rog, 2013-03-27 11:10:17]

LGTM when william's happy.

https://codereview.appspot.com/8034043/diff/5001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/8034043/diff/5001/state/service.go#newcode311
state/service.go:311: func (s *Service) peerRelationsDifference(otherMeta
*charm.Meta) map[string]charm.Relation {
On 2013/03/27 09:49:47, fwereade wrote:
> consider:
> 
> s/peerRelationsDifference/newPeerRelations/

the problem with "new" in that context is it
look like a constructor.

newerPeerRelations?

[dimitern, 2013-03-27 11:26:27]

Please take a look.

https://codereview.appspot.com/8034043/diff/5001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/8034043/diff/5001/state/service.go#newcode311
state/service.go:311: func (s *Service) peerRelationsDifference(otherMeta
*charm.Meta) map[string]charm.Relation {
On 2013/03/27 11:10:17, rog wrote:
> On 2013/03/27 09:49:47, fwereade wrote:
> > consider:
> > 
> > s/peerRelationsDifference/newPeerRelations/
> 
> the problem with "new" in that context is it
> look like a constructor.
> 
> newerPeerRelations?

compromise: extraPeerRelations :)

https://codereview.appspot.com/8034043/diff/5001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/8034043/diff/5001/state/service_test.go#newcod...
state/service_test.go:297: c.Assert(ch.Meta().Peers, HasLen, 2)
On 2013/03/27 09:49:47, fwereade wrote:
> I think the tests down to here are redundant -- the SetCharm tests do already
> test that we set the correct charm on the service, and all the
> Service.Endpoint() stuff is tested pretty comprehensively elsewhere.

see below.

https://codereview.appspot.com/8034043/diff/5001/state/service_test.go#newcod...
state/service_test.go:298: c.Assert(state.ServiceRelationCount(s.mysql), Equals,
2)
On 2013/03/27 09:49:47, fwereade wrote:
> I would much prefer a black-box test that demonstrated correct behaviour on
> Destroy.

Changed as discussed; black-box testing + assert on relations, rather than
endpoints.

https://codereview.appspot.com/8034043/diff/5001/state/service_test.go#newcod...
state/service_test.go:302: ep, err = rels[0].Endpoint("mysql")
On 2013/03/27 09:49:47, fwereade wrote:
> I don't think it's appropriate to assume an ordering for the result of
> Relations() here.

The tests were changed as discussed.

[fwereade, 2013-03-27 11:45:00]

LGTM with trivials

https://codereview.appspot.com/8034043/diff/13001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/8034043/diff/13001/state/service.go#newcode314
state/service.go:314: panic("newMeta is nil; cannot determine new peer
relations")
I wouldn't even bother with this, the usual panic should give us enough context.

https://codereview.appspot.com/8034043/diff/13001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/8034043/diff/13001/state/service_test.go#newco...
state/service_test.go:257: func (s *ServiceSuite) assertServiceRelations(c *C,
svc *state.Service, expectedKeys []string) []*state.Relation {
consider `expectedKeys ...string` for slightly neater usage at call sites

https://codereview.appspot.com/8034043/diff/13001/state/service_test.go#newco...
state/service_test.go:260: c.Assert(rels, HasLen, len(expectedKeys))
Maybe drop the len checks; just sort both string slices and DeepEquals them.
(clearer errors when they go wrong)

https://codereview.appspot.com/8034043/diff/13001/state/service_test.go#newco...
state/service_test.go:288: 
// Check state consistency by attempting to destroy the service.

[dimitern, 2013-03-27 12:00:56]

*** Submitted:

state: add new peer relations on upgrade

When setiing a serivce's charm on upgrade, new
peer relations are now added as part of the same
transaction.

Slight refactoring of AddService is done as well,
extracting common code now used by both AS() and
service.SetCharm.

Refactored testing Add*Charm() calls to use more
generic code. In a follow-up it'll be improved
further.

R=rog, fwereade
CC=
https://codereview.appspot.com/8034043

https://codereview.appspot.com/8034043/diff/13001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/8034043/diff/13001/state/service.go#newcode314
state/service.go:314: panic("newMeta is nil; cannot determine new peer
relations")
On 2013/03/27 11:45:00, fwereade wrote:
> I wouldn't even bother with this, the usual panic should give us enough
context.

Done.

https://codereview.appspot.com/8034043/diff/13001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/8034043/diff/13001/state/service_test.go#newco...
state/service_test.go:257: func (s *ServiceSuite) assertServiceRelations(c *C,
svc *state.Service, expectedKeys []string) []*state.Relation {
On 2013/03/27 11:45:00, fwereade wrote:
> consider `expectedKeys ...string` for slightly neater usage at call sites

Done.

https://codereview.appspot.com/8034043/diff/13001/state/service_test.go#newco...
state/service_test.go:260: c.Assert(rels, HasLen, len(expectedKeys))
On 2013/03/27 11:45:00, fwereade wrote:
> Maybe drop the len checks; just sort both string slices and DeepEquals them.
> (clearer errors when they go wrong)

Done.

https://codereview.appspot.com/8034043/diff/13001/state/service_test.go#newco...
state/service_test.go:288: 
On 2013/03/27 11:45:00, fwereade wrote:
> // Check state consistency by attempting to destroy the service.

Done.