Fix state relation interface

Fix state relation interface

Issues that became apparent when trying to use it:

* we need to know what relations a service is participating in
* the Relation type is entirely pointless
* there's no reason to return anything other than an error from AddRelation
* there's no reason for RemoveRelation to take any arguments other than the
  endpoint list (as in AddRelation)

https://code.launchpad.net/~fwereade/juju-core/convenient-relation-interface/+merge/109377

(do not edit description out of merge proposal)

[fwereade, 2012-06-08 14:57:51]

Please take a look.

[TheMue, 2012-06-08 16:00:17]

LGTM, will only conflict with my validation branch. Has then to be adopted.

[fwereade, 2012-06-14 17:18:50]

Please take a look.

[niemeyer, 2012-06-15 00:48:13]

Wow, that's a significant simplification to the API. Thanks for pushing these
ideas forward.

A few details for your consideration:

https://codereview.appspot.com/6303060/diff/6001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6303060/diff/6001/state/service.go#newcode224
state/service.go:224: func (s *Service) Relations() ([]*ServiceRelation, error)
{
I guess this needs something along the lines of:

defer errorContextf(&err, "can't get relations for service %q", s.name)

https://codereview.appspot.com/6303060/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6303060/diff/6001/state/state.go#newcode274
state/state.go:274: // for the newly created node.
This is obscuring a bit what this method's primary purpose is. I suggest
something like:

// addRelationNode creates the node for the relation represented by the
// given endpoints, and returns the node name to be used as a relation key.
// The provided endpoints are validated before the relation node is created.

https://codereview.appspot.com/6303060/diff/6001/state/state.go#newcode275
state/state.go:275: func (s *State) addRelationNode(endpoints
...RelationEndpoint) (string, error) {
s/string, /relationKey string, err/

It's nice to use named results when the type doesn't help. Gives a good idea of
what the function returns in a glimpse.

https://codereview.appspot.com/6303060/diff/6001/state/state.go#newcode349
state/state.go:349: // RemoveRelation removes any extant relation between the
given endpoints.
s/any extant/the/

There can't be more than one, and that one is well defined by the specific
endpoints provided.

https://codereview.appspot.com/6303060/diff/6001/state/state.go#newcode351
state/state.go:351: defer errorContextf(&err, "can't remove relation %s",
describeRelation(endpoints))
I'm feeling a bit concerned about the overuse of errorContextf. This is a case
that would be trivial to handle with the usual fmt.Errorf convention. In fact,
it was already being done that way, and looks quite ok. That'd also avoid the
unnecessary use of describeRelation to build a value that is going to be
discarded in most cases.

https://codereview.appspot.com/6303060/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6303060/diff/6001/state/state_test.go#newcode1278
state/state_test.go:1278: c.Assert(err, ErrorMatches, `can't add relation
<peer:baz, req:bar>: endpoints do not relate`)
Wouldn't "peer:baz req:bar" (with the quotes) be more readable? That's how the
user types that information in the command line.

[rog, 2012-06-15 06:17:07]

LGTM modulo gustavo's comments

https://codereview.appspot.com/6303060/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6303060/diff/6001/state/state.go#newcode275
state/state.go:275: func (s *State) addRelationNode(endpoints
...RelationEndpoint) (string, error) {
On 2012/06/15 00:48:14, niemeyer wrote:
> s/string, /relationKey string, err/
> 
> It's nice to use named results when the type doesn't help. Gives a good idea
of
> what the function returns in a glimpse.

+1. especially as the type signature is printed in the godoc summary.

[fwereade, 2012-06-15 10:55:47]

Please take a look.

https://codereview.appspot.com/6303060/diff/6001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6303060/diff/6001/state/service.go#newcode224
state/service.go:224: func (s *Service) Relations() ([]*ServiceRelation, error)
{
On 2012/06/15 00:48:14, niemeyer wrote:
> I guess this needs something along the lines of:
> 
> defer errorContextf(&err, "can't get relations for service %q", s.name)

Done.

https://codereview.appspot.com/6303060/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6303060/diff/6001/state/state.go#newcode274
state/state.go:274: // for the newly created node.
On 2012/06/15 00:48:14, niemeyer wrote:
> This is obscuring a bit what this method's primary purpose is. I suggest
> something like:
> 
> // addRelationNode creates the node for the relation represented by the
> // given endpoints, and returns the node name to be used as a relation key.
> // The provided endpoints are validated before the relation node is created.

Done.

https://codereview.appspot.com/6303060/diff/6001/state/state.go#newcode275
state/state.go:275: func (s *State) addRelationNode(endpoints
...RelationEndpoint) (string, error) {
On 2012/06/15 06:17:07, rog wrote:
> On 2012/06/15 00:48:14, niemeyer wrote:
> > s/string, /relationKey string, err/
> > 
> > It's nice to use named results when the type doesn't help. Gives a good idea
> of
> > what the function returns in a glimpse.
> 
> +1. especially as the type signature is printed in the godoc summary.

Done.

https://codereview.appspot.com/6303060/diff/6001/state/state.go#newcode349
state/state.go:349: // RemoveRelation removes any extant relation between the
given endpoints.
On 2012/06/15 00:48:14, niemeyer wrote:
> s/any extant/the/
> 
> There can't be more than one, and that one is well defined by the specific
> endpoints provided.

Done.

https://codereview.appspot.com/6303060/diff/6001/state/state.go#newcode351
state/state.go:351: defer errorContextf(&err, "can't remove relation %s",
describeRelation(endpoints))
On 2012/06/15 00:48:14, niemeyer wrote:
> I'm feeling a bit concerned about the overuse of errorContextf. This is a case
> that would be trivial to handle with the usual fmt.Errorf convention. In fact,
> it was already being done that way, and looks quite ok. That'd also avoid the
> unnecessary use of describeRelation to build a value that is going to be
> discarded in most cases.

Done.

https://codereview.appspot.com/6303060/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6303060/diff/6001/state/state_test.go#newcode1278
state/state_test.go:1278: c.Assert(err, ErrorMatches, `can't add relation
<peer:baz, req:bar>: endpoints do not relate`)
On 2012/06/15 00:48:14, niemeyer wrote:
> Wouldn't "peer:baz req:bar" (with the quotes) be more readable? That's how the
> user types that information in the command line.

Done.

[niemeyer, 2012-06-15 22:34:23]

LGTM. Just a trivial that was introduced since the last review:

https://codereview.appspot.com/6303060/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6303060/diff/6001/state/state_test.go#newcode1278
state/state_test.go:1278: c.Assert(err, ErrorMatches, `can't add relation
<peer:baz, req:bar>: endpoints do not relate`)
On 2012/06/15 10:55:47, fwereade wrote:
> > Wouldn't "peer:baz req:bar" (with the quotes) be more readable? That's how
the
> > user types that information in the command line.
> 
> Done.

Thanks. These messages seem to look better now indeed.

https://codereview.appspot.com/6303060/diff/6007/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6303060/diff/6007/state/service.go#newcode224
state/service.go:224: func (s *Service) Relations() (serviceRelation
[]*ServiceRelation, err error) {
No need for the named results here. They are unused in the function, and the
type names are clear (actually more clear than the variable names.. the first
one should be plural, if it was really used).

[fwereade, 2012-06-18 07:02:09]

*** Submitted:

Fix state relation interface

Issues that became apparent when trying to use it:

* we need to know what relations a service is participating in
* the Relation type is entirely pointless
* there's no reason to return anything other than an error from AddRelation
* there's no reason for RemoveRelation to take any arguments other than the
  endpoint list (as in AddRelation)

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