Code review comment for lp:~themue/pyjuju/go-state-topology-relation-endpoints

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

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.

https://codereview.appspot.com/6198055/

« Back to merge proposal