Added methods for relation endpoints to topology.

Added methods for relation endpoints to topology.

Relation endpoints needed two methods for testing and
key retrieving in topology.

https://code.launchpad.net/~themue/juju/go-state-topology-relation-endpoints/+merge/105156

Requires: https://code.launchpad.net/~themue/juju/go-state-topology-relations-without-endpoints/+merge/104765

(do not edit description out of merge proposal)

[TheMue, 2012-05-09 01:09:03]

Please take a look.

[fwereade, 2012-05-09 18:25:35]

https://codereview.appspot.com/6198055/diff/1/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/1/state/topology.go#newcode550
state/topology.go:550: if service == nil || service.Name != e.RelationName ||
scope != e.RelationScope {
I'm pretty sure that "service.Name != e.RelationName" is either insane, or it
betrays a serious misnaming issue somewhere.

That said, per prereq review, I think we should drop RelationName anyway.

https://codereview.appspot.com/6198055/diff/1/state/topology.go#newcode563
state/topology.go:563: // between "endpoints" or "found" will be false.
I'm -1 on the ... here, I think it's a wart in the python version and I'd prefer
not to duplicate it in Go. Also, it feels like "found" is redundant -- can't we
just return a key of ""? How about:

func (t *topology) RelationKey(ep1, ep2 RelationEndpoint) (string, error)

func (t *topology) PeerRelationKey(ep RelationEndpoint) (string, error)

...and, given that, I think that HasRelationBetweenEndpoints is actually
completely redundant.

https://codereview.appspot.com/6198055/diff/1/state/topology.go#newcode576
state/topology.go:576: for key, relation := range t.topology.Relations {
Please double-check why the implementation here is different to the one in
HasRelationBetweenEndpoints... by gut says that this is evidence of a bug
somewhere.

[TheMue, 2012-05-14 18:09:13]

Please take a look.

https://codereview.appspot.com/6198055/diff/1/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/1/state/topology.go#newcode550
state/topology.go:550: if service == nil || service.Name != e.RelationName ||
scope != e.RelationScope {
On 2012/05/09 18:25:35, fwereade wrote:
> I'm pretty sure that "service.Name != e.RelationName" is either insane, or it
> betrays a serious misnaming issue somewhere.
> 
> That said, per prereq review, I think we should drop RelationName anyway.

Obsolete due to refactoring.

https://codereview.appspot.com/6198055/diff/1/state/topology.go#newcode563
state/topology.go:563: // between "endpoints" or "found" will be false.
On 2012/05/09 18:25:35, fwereade wrote:
> I'm -1 on the ... here, I think it's a wart in the python version and I'd
prefer
> not to duplicate it in Go. Also, it feels like "found" is redundant -- can't
we
> just return a key of ""? How about:
> 
> func (t *topology) RelationKey(ep1, ep2 RelationEndpoint) (string, error)
> 
> func (t *topology) PeerRelationKey(ep RelationEndpoint) (string, error)
> 
> ...and, given that, I think that HasRelationBetweenEndpoints is actually
> completely redundant.

Done, looks better now.

https://codereview.appspot.com/6198055/diff/1/state/topology.go#newcode576
state/topology.go:576: for key, relation := range t.topology.Relations {
On 2012/05/09 18:25:35, fwereade wrote:
> Please double-check why the implementation here is different to the one in
> HasRelationBetweenEndpoints... by gut says that this is evidence of a bug
> somewhere.

Obsolete changed due to refactoring.

[niemeyer, 2012-05-17 19:16:45]

https://codereview.appspot.com/6198055/diff/5001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/5001/state/topology.go#newcode545
state/topology.go:545: return "", nil
If the developer asks for a RelationKey, and it doesn't exist, an error should
be returned. Silently returning nothing and hoping that the developer remembers
to check for the case where it doesn't exist every time is extremely error
prone.

Imagine if you had to do this:

    f, err := os.Open(filename)
    if err != nil {
         ...
    }
    if f != nil {
         ...
    }
    ...

Every time!

The documentation should reflect the change too.

https://codereview.appspot.com/6198055/diff/5001/state/topology.go#newcode571
state/topology.go:571: return "", nil
Ditto.

[TheMue, 2012-05-21 14:16:13]

Please take a look.

[TheMue, 2012-05-21 15:25:18]

Please take a look.

[niemeyer, 2012-05-21 17:39:25]

https://codereview.appspot.com/6198055/diff/8002/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/8002/state/topology.go#newcode20
state/topology.go:20: errRelationDoesNotExist = errors.New("relation does not
exist")
If we define that error more nicely, we can allow it to pop out to of the state
package when using it. For example:

type noRelationError struct {
    endpoint1, endpoint2 RelationEndpoint
}

func (e *noRelationError) Error() string {
    if e.endpoint2 != nil {
        return fmt.Errorf("state: no peer relation for %s", (what here?)) 
    } else {
        return fmt.Errorf("state: no relation between %s and %s", (what here?))
    }
}

Not sure about the details of the error messages, but it should be something
nice that defines the endpoints.

What do you think?

[TheMue, 2012-05-21 17:43:19]

On 2012/05/21 17:39:25, niemeyer wrote:
> https://codereview.appspot.com/6198055/diff/8002/state/topology.go
> File state/topology.go (right):
> 
> https://codereview.appspot.com/6198055/diff/8002/state/topology.go#newcode20
> state/topology.go:20: errRelationDoesNotExist = errors.New("relation does not
> exist")
> If we define that error more nicely, we can allow it to pop out to of the
state
> package when using it. For example:
> 
> type noRelationError struct {
>     endpoint1, endpoint2 RelationEndpoint
> }
> 
> func (e *noRelationError) Error() string {
>     if e.endpoint2 != nil {
>         return fmt.Errorf("state: no peer relation for %s", (what here?)) 
>     } else {
>         return fmt.Errorf("state: no relation between %s and %s", (what
here?))
>     }
> }
> 
> Not sure about the details of the error messages, but it should be something
> nice that defines the endpoints.
> 
> What do you think?

Good idea. I would even export the endpoints. So the receiver of that error can
analyze it more than only print the message.

[TheMue, 2012-05-23 11:55:52]

Please take a look.

[niemeyer, 2012-05-23 23:14:39]

Also in a good direction, thanks Frank.

https://codereview.appspot.com/6198055/diff/9002/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6198055/diff/9002/state/relation.go#newcode55
state/relation.go:55: func (e *RelationEndpoint) String() string {
That's somewhat unexpected. What's the goal here?

I think I'd prefer to leave this out. %#v and %v are simpler and produce better
results than this as it is.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode17
state/topology.go:17: // consumer endpoints of for one peer endpoint does not
exist.
// NoRelationError represents a relation not found for one or more endpoints.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode19
state/topology.go:19: Endpoint1 *RelationEndpoint
I suggest using this instead:

    Endpoints []RelationEndpoint

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode474
state/topology.go:474: // interface between two endpoints. If it doesn't exist
an error is returned.
// RelationKey returns the key for the relation established between the
// provided endpoints. If no matching relation is found, error will be
// of type *NoRelationError.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode475
state/topology.go:475: func (t *topology) RelationKey(ep1, ep2 RelationEndpoint)
(string, error) {
I think the design we had is better one here:

RelationKey(endpoints ...RelationEndpoint)

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode494
state/topology.go:494: if ep1.Interface != relation.Interface || ep2.Interface
!= relation.Interface {
If ep1 and ep2 have diverging interfaces, we can return an error before even
starting to search.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode499
state/topology.go:499: }
Relation names matter as well.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode506
state/topology.go:506: func (t *topology) PeerRelationKey(ep RelationEndpoint)
(string, error) {
We can subsume that into RelationKey as well.

[niemeyer, 2012-05-24 21:50:54]

Moving it back onto WIP.

[TheMue, 2012-05-25 15:41:03]

Please take a look.

https://codereview.appspot.com/6198055/diff/9002/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6198055/diff/9002/state/relation.go#newcode55
state/relation.go:55: func (e *RelationEndpoint) String() string {
On 2012/05/23 23:14:39, niemeyer wrote:
> That's somewhat unexpected. What's the goal here?
> 
> I think I'd prefer to leave this out. %#v and %v are simpler and produce
better
> results than this as it is.

Done.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode17
state/topology.go:17: // consumer endpoints of for one peer endpoint does not
exist.
On 2012/05/23 23:14:39, niemeyer wrote:
> // NoRelationError represents a relation not found for one or more endpoints.

Done.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode19
state/topology.go:19: Endpoint1 *RelationEndpoint
On 2012/05/23 23:14:39, niemeyer wrote:
> I suggest using this instead:
> 
>     Endpoints []RelationEndpoint

Done.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode474
state/topology.go:474: // interface between two endpoints. If it doesn't exist
an error is returned.
On 2012/05/23 23:14:39, niemeyer wrote:
> // RelationKey returns the key for the relation established between the
> // provided endpoints. If no matching relation is found, error will be
> // of type *NoRelationError.

Done.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode475
state/topology.go:475: func (t *topology) RelationKey(ep1, ep2 RelationEndpoint)
(string, error) {
On 2012/05/23 23:14:39, niemeyer wrote:
> I think the design we had is better one here:
> 
> RelationKey(endpoints ...RelationEndpoint)

Done.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode494
state/topology.go:494: if ep1.Interface != relation.Interface || ep2.Interface
!= relation.Interface {
On 2012/05/23 23:14:39, niemeyer wrote:
> If ep1 and ep2 have diverging interfaces, we can return an error before even
> starting to search.

Done.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode499
state/topology.go:499: }
On 2012/05/23 23:14:39, niemeyer wrote:
> Relation names matter as well.

Done.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode506
state/topology.go:506: func (t *topology) PeerRelationKey(ep RelationEndpoint)
(string, error) {
On 2012/05/23 23:14:39, niemeyer wrote:
> We can subsume that into RelationKey as well.

Done.

[TheMue, 2012-05-25 16:49:23]

Please take a look.

[TheMue, 2012-05-25 18:02:13]

Please take a look.

[niemeyer, 2012-05-25 18:25:22]

https://codereview.appspot.com/6198055/diff/27001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/27001/state/topology.go#newcode471
state/topology.go:471: return "", fmt.Errorf("state: differing interfaces %q and
%q", endpoints[0].Interface, endpoints[1].Interface)
What about just returning NoRelationError?

https://codereview.appspot.com/6198055/diff/27001/state/topology.go#newcode474
state/topology.go:474: return "", fmt.Errorf("state: illegal number of endpoints
passed")
s/passed/provided/

https://codereview.appspot.com/6198055/diff/27001/state/topology.go#newcode491
state/topology.go:491: if serviceName != endpoint.RelationName {
Huh? That's quite wrong.

[TheMue, 2012-05-25 20:55:45]

Please take a look.

https://codereview.appspot.com/6198055/diff/27001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/27001/state/topology.go#newcode471
state/topology.go:471: return "", fmt.Errorf("state: differing interfaces %q and
%q", endpoints[0].Interface, endpoints[1].Interface)
On 2012/05/25 18:25:23, niemeyer wrote:
> What about just returning NoRelationError?

Done.

https://codereview.appspot.com/6198055/diff/27001/state/topology.go#newcode474
state/topology.go:474: return "", fmt.Errorf("state: illegal number of endpoints
passed")
On 2012/05/25 18:25:23, niemeyer wrote:
> s/passed/provided/

Done.

[niemeyer, 2012-05-29 17:22:14]

LGTM, with a few trivials:

https://codereview.appspot.com/6198055/diff/25003/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode29
state/topology.go:29: panic("state: illegal relation error")
s/ error//

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode73
state/topology.go:73: ServiceKey   string
s/ServiceKey/Service/; see the convention in the other types.

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode74
state/topology.go:74: RelationName string
Please add a tag "relation-name" so we follow the naming convention too.

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode430
state/topology.go:430: return fmt.Errorf("provider and consumer keys must not be
the same")
s/consumer/requirer/

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode474
state/topology.go:474: if t.topology.Relations == nil {
Shouldn't the logic below work fine if this is dropped?

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode485
state/topology.go:485: return "", fmt.Errorf("state: illegal number of endpoints
provided")
s/endpoints/relation endpoints/

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode498
state/topology.go:498: if service.RelationName != endpoint.RelationName {
Both of these if blocks can be made into one:

if !ok || ... {

[TheMue, 2012-05-29 18:01:46]

Please take a look.

https://codereview.appspot.com/6198055/diff/25003/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode29
state/topology.go:29: panic("state: illegal relation error")
On 2012/05/29 17:22:15, niemeyer wrote:
> s/ error//

Done.

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode73
state/topology.go:73: ServiceKey   string
On 2012/05/29 17:22:15, niemeyer wrote:
> s/ServiceKey/Service/; see the convention in the other types.

Done.

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode74
state/topology.go:74: RelationName string
On 2012/05/29 17:22:15, niemeyer wrote:
> Please add a tag "relation-name" so we follow the naming convention too.

Done.

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode430
state/topology.go:430: return fmt.Errorf("provider and consumer keys must not be
the same")
On 2012/05/29 17:22:15, niemeyer wrote:
> s/consumer/requirer/

Done. Aargh, a reflex, too often written provider together with consumer. ;)

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode474
state/topology.go:474: if t.topology.Relations == nil {
On 2012/05/29 17:22:15, niemeyer wrote:
> Shouldn't the logic below work fine if this is dropped?

Done.

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode485
state/topology.go:485: return "", fmt.Errorf("state: illegal number of endpoints
provided")
On 2012/05/29 17:22:15, niemeyer wrote:
> s/endpoints/relation endpoints/

Done.

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode498
state/topology.go:498: if service.RelationName != endpoint.RelationName {
On 2012/05/29 17:22:15, niemeyer wrote:
> Both of these if blocks can be made into one:
> 
> if !ok || ... {

Done.

[niemeyer, 2012-05-29 18:06:38]

LGTM

[TheMue, 2012-05-30 07:59:51]

*** Submitted:

Added methods for relation endpoints to topology.

Relation endpoints needed two methods for testing and
key retrieving in topology.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6198055