state: Added the method RemoveRelation() to State.

state: Added the method RemoveRelation() to State.

Relation is now an interface defining the single
method relationKey() to allow a function signature
like today in Python. The implementation is now
named relation and can be accessed via type
assertion.

https://code.launchpad.net/~themue/juju/go-state-remove-relation/+merge/108037

Requires: https://code.launchpad.net/~themue/juju/go-state-add-relation/+merge/106652

(do not edit description out of merge proposal)

[TheMue, 2012-05-30 18:52:18]

Please take a look.

[niemeyer, 2012-05-30 22:40:33]

https://codereview.appspot.com/6250076/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6250076/diff/1/state/relation.go#newcode52
state/relation.go:52: relationKey() string
Why is this being turned into an interface that has a single implementation and
no chance of ever being implemented by anyone outside of this package?

I've read the description of the CL, but it still makes no sense to me.

[TheMue, 2012-05-31 09:26:30]

> I've read the description of the CL, but it still makes no sense to me.

Maybe too short, sorry. Todays remove_relation_state gets a relation to remove
it from topology. It's only interested in the internal id and uses isinstance()
to check the type.

So far the Go implementation used two different types. To realize a similar
function signature [RemoveRelation(rel Relation)] I needed an interface
implemented by both (and later maybe more) types to do the type assertion and
then get the internal key (that's why its only method is package private). 

I could have let the name Relation for the more general relation type (which in
Python only contains the internal id) but found no good name for the interface.
Here Relation is the most natural one. Is there any needance to implement a
relation outside of the package?

Any other solution idea is welcome. Due to the simple logic it can also be split
into the two methods RemoveRelation() and RemoveServiceRelation().

[niemeyer, 2012-05-31 10:22:16]

> Any other solution idea is welcome. Due to the simple logic it can also be
split
> into the two methods RemoveRelation() and RemoveServiceRelation().

+1

[niemeyer, 2012-05-31 10:23:15]

On 2012/05/31 10:22:16, niemeyer wrote:
> > Any other solution idea is welcome. Due to the simple logic it can also be
> split
> > into the two methods RemoveRelation() and RemoveServiceRelation().
> 
> +1

Actually, sorry, that's misleading. I suggest adding a Relation() method to the
ServiceRelation type.

[TheMue, 2012-06-01 10:34:58]

Please take a look.

[TheMue, 2012-06-01 10:40:20]

Please take a look.

[TheMue, 2012-06-01 11:14:35]

Please take a look.

[niemeyer, 2012-06-01 11:32:17]

<niemeyer> TheMue: I see you're renaming from zkRelation to topoRelation.. are
tests broken at the moment without that?
<TheMue> niemeyer: Without renaming it has been broken.
<TheMue> niemeyer: Yes.
<niemeyer> TheMue: Ok :(
<niemeyer> TheMue: Trunk is being broken way too often :(
<niemeyer>   382                 t.RemoveRelation(relation.key)
<niemeyer>  383                 return nil
<niemeyer> TheMue: Is RemoveRelation returning no errorr?
<TheMue> niemeyer: No, it's only removing inside a map. Today it's also not
checked at that point if it exists in that dir.
<niemeyer> TheMue: OK
<niemeyer> / Relation returns the relation with key from the topology.
<niemeyer> func (t *topology) Relation(key string) (*topoRelation, error) {
<niemeyer>         if t.topology.Relations == nil || t.topology.Relations[key]
== nil {
<niemeyer>                 return nil, fmt.Errorf("relation %q does not exist",
key)
<niemeyer> TheMue: We don't want the user to be getting this error message
<TheMue> niemeyer: Oh, please wait.Just seen an assert_relation there. Will add
it too.
<niemeyer> TheMue: The relation key is completely uninteresting as far as a user
is concerned
<niemeyer> TheMue: The state method should verify the state in those cases, and
error early with a sensible message
<TheMue> niemeyer: OK
<niemeyer> TheMue: If we get an Relation from relation we should display
prefixed with something saying that an unexpected action happened while doing
whatever
<niemeyer> TheMue: In which case the relation key might go out, but we have no
idea about why it was failing since we've checked it early
<niemeyer> TheMue: So it's debugging rather than something we'd like the user to
be commonly looking at
<TheMue> niemeyer: IC. Do you note it in the review?
<niemeyer> TheMue: I'll copy & paste what I just said

[TheMue, 2012-06-01 14:25:53]

Please take a look.

[niemeyer, 2012-06-05 20:35:40]

https://codereview.appspot.com/6250076/diff/6032/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6250076/diff/6032/state/relation.go#newcode84
state/relation.go:84: func (r *ServiceRelation) Relation() *Relation {
This needs a test.

https://codereview.appspot.com/6250076/diff/6032/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6250076/diff/6032/state/state_test.go#newcode1136
state/state_test.go:1136: func (s *StateSuite) TestRemoveRServiceRelation(c *C)
{
Please drop this test. RemoveRelation is tested above. We don't have to test
that RemoveRelation works with the result of ServiceRelation.Relation. We have
to test that ServiceRelation.Relation, by itself, works.

https://codereview.appspot.com/6250076/diff/6032/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6250076/diff/6032/state/topology.go#newcode403
state/topology.go:403: return nil, fmt.Errorf("relation does not exist")
Please revert this change. The goal isn't to prevent this from showing the key.
The goal is to make sure that the high level layer presents a *good* high-level
error.

Going through the state code I see that errors continue to be all over the
place. We've talked about this before, and didn't really do it. Now may be the
right time to stop and go back to fix our debt in the error management of the
state code.

We can go ahead and integrate this, but please do revert this change so that it
continues to show the proper key in the low-level interface error.

In the high-level interface, please add a TODO so we remember it's broken.

[niemeyer, 2012-06-05 20:54:27]

By the way, as a suggestion for testing ServiceRelation.Relation, just a simple
HasRelation check with a positive and a negative test should do.

[TheMue, 2012-06-06 12:12:04]

Please take a look.

https://codereview.appspot.com/6250076/diff/6032/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6250076/diff/6032/state/relation.go#newcode84
state/relation.go:84: func (r *ServiceRelation) Relation() *Relation {
On 2012/06/05 20:35:40, niemeyer wrote:
> This needs a test.

Done. It's now in TestServiceRelationRelation() using HasRelation().

https://codereview.appspot.com/6250076/diff/6032/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6250076/diff/6032/state/state_test.go#newcode1136
state/state_test.go:1136: func (s *StateSuite) TestRemoveRServiceRelation(c *C)
{
On 2012/06/05 20:35:40, niemeyer wrote:
> Please drop this test. RemoveRelation is tested above. We don't have to test
> that RemoveRelation works with the result of ServiceRelation.Relation. We have
> to test that ServiceRelation.Relation, by itself, works.

Done.

[niemeyer, 2012-06-06 12:32:56]

Unfortunately the CL is dirty again with unrelated changes.

Please let me know once it's ready for another look.

[TheMue, 2012-06-06 12:55:48]

Please take a look.

[niemeyer, 2012-06-06 13:32:10]

LGTM, thanks. Just a trivial before submitting:

https://codereview.appspot.com/6250076/diff/17003/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6250076/diff/17003/state/state.go#newcode377
state/state.go:377: // TODO: Improve high-level errors, no low-level passing.
Please move this to within the function. Doesn't make sense to have it exposed
in the API documentation.

[TheMue, 2012-06-06 13:44:16]

*** Submitted:

state: Added the method RemoveRelation() to State.

Relation is now an interface defining the single
method relationKey() to allow a function signature
like today in Python. The implementation is now
named relation and can be accessed via type
assertion.

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