Merge lp:~themue/pyjuju/go-state-remove-relation into lp:pyjuju/go

[Frank Mueller]

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://codereview.appspot.com/6250076/

[Gustavo Niemeyer (niemeyer)]

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.

https://codereview.appspot.com/6250076/

[Gustavo Niemeyer (niemeyer)]

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

+1

https://codereview.appspot.com/6250076/

[Gustavo Niemeyer (niemeyer)]

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.

https://codereview.appspot.com/6250076/

[Gustavo Niemeyer (niemeyer)]

<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

https://codereview.appspot.com/6250076/

[Gustavo Niemeyer (niemeyer)]

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.

https://codereview.appspot.com/6250076/

[Gustavo Niemeyer (niemeyer)]

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

https://codereview.appspot.com/6250076/

[Gustavo Niemeyer (niemeyer)]

Unfortunately the CL is dirty again with unrelated changes.

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

https://codereview.appspot.com/6250076/

[Gustavo Niemeyer (niemeyer)]

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.

https://codereview.appspot.com/6250076/

=== modified file 'state/export_test.go'
--- state/export_test.go	2012-05-28 23:15:30 +0000
+++ state/export_test.go	2012-06-06 13:08:23 +0000
@@ -1,6 +1,3 @@
-// launchpad.net/juju/state
-//
-// Copyright (c) 2011-2012 Canonical Ltd.
 package state
 
 import (
@@ -27,4 +24,17 @@
 	return readConfigNode(st.zk, path)
 }
 
+// HasRelation tests if the topology contains a relation.
+func HasRelation(st *State, relation *Relation) (bool, error) {
+	t, err := readTopology(st.zk)
+	if err != nil {
+		return false, err
+	}
+	_, err = t.Relation(relation.key)
+	if err != nil {
+		return false, err
+	}
+	return true, nil
+}
+
 func Diff(a, b []string) []string { return diff(a, b) }
 
=== modified file 'state/relation.go'
--- state/relation.go	2012-05-31 16:19:15 +0000
+++ state/relation.go	2012-06-06 13:08:23 +0000
@@ -79,3 +79,8 @@
 func (r *ServiceRelation) RelationName() string {
 	return r.relationName
 }
+
+// Relation returns the relation for this service relation.
+func (r *ServiceRelation) Relation() *Relation {
+	return &Relation{r.st, r.relationKey}
+}
 
=== modified file 'state/state.go'
--- state/state.go	2012-06-05 21:31:50 +0000
+++ state/state.go	2012-06-06 13:08:23 +0000
@@ -372,3 +372,16 @@
 	}
 	return &Relation{s, relationKey}, serviceRelations, nil
 }
+
+// RemoveRelation removes the relation.
+// TODO: Improve high-level errors, no low-level passing.
+func (s *State) RemoveRelation(relation *Relation) error {
+	removeRelation := func(t *topology) error {
+		_, err := t.Relation(relation.key)
+		if err != nil {
+			return fmt.Errorf("can't remove relation: %v", err)
+		}
+		return t.RemoveRelation(relation.key)
+	}
+	return retryTopologyChange(s.zk, removeRelation)
+}
 
=== modified file 'state/state_test.go'
--- state/state_test.go	2012-06-06 10:17:40 +0000
+++ state/state_test.go	2012-06-06 13:08:23 +0000
@@ -1085,7 +1085,24 @@
 	c.Assert(serviceRelations[0].RelationScope(), Equals, state.ScopeGlobal)
 	c.Assert(serviceRelations[0].RelationRole(), Equals, state.RolePeer)
 	c.Assert(serviceRelations[0].RelationName(), Equals, "cache")
+}
 
+func (s *StateSuite) TestServiceRelationRelation(c *C) {
+	dummy := s.addDummyCharm(c)
+	s.st.AddService("riak", dummy)
+	riakep := state.RelationEndpoint{"riak", "ring", "cache", state.RolePeer, state.ScopeGlobal}
+	relation, serviceRelations, err := s.st.AddRelation(riakep)
+	c.Assert(err, IsNil)
+	c.Assert(relation, NotNil)
+	c.Assert(serviceRelations, HasLen, 1)
+	ok, err := state.HasRelation(s.st, serviceRelations[0].Relation())
+	c.Assert(err, IsNil)
+	c.Assert(ok, Equals, true)
+	// Negative test.
+	err = s.st.RemoveRelation(relation)
+	c.Assert(err, IsNil)
+	_, err = state.HasRelation(s.st, serviceRelations[0].Relation())
+	c.Assert(err, ErrorMatches, `relation "relation-0000000000" does not exist`)
 }
 
 func (s *StateSuite) TestAddRelationMissingService(c *C) {
@@ -1169,3 +1186,21 @@
 	c.Assert(err, IsNil)
 	c.Assert(env.Map(), DeepEquals, map[string]interface{}{"type": "dummy", "name": "foo"})
 }
+
+func (s *StateSuite) TestRemoveRelation(c *C) {
+	dummy := s.addDummyCharm(c)
+	s.st.AddService("riak", dummy)
+	riakep := state.RelationEndpoint{"riak", "blog", "cache", state.RolePeer, state.ScopeGlobal}
+	relation, _, err := s.st.AddRelation(riakep)
+	c.Assert(err, IsNil)
+	c.Assert(relation, NotNil)
+	hasRelation, err := state.HasRelation(s.st, relation)
+	c.Assert(err, IsNil)
+	c.Assert(hasRelation, Equals, true)
+	err = s.st.RemoveRelation(relation)
+	hasRelation, err = state.HasRelation(s.st, relation)
+	c.Assert(hasRelation, Equals, false)
+	c.Assert(err, ErrorMatches, `relation "relation-0000000000" does not exist`)
+	err = s.st.RemoveRelation(relation)
+	c.Assert(err, ErrorMatches, `can't remove relation: relation "relation-0000000000" does not exist`)
+}
 
=== modified file 'state/topology.go'
--- state/topology.go	2012-05-31 09:48:26 +0000
+++ state/topology.go	2012-06-06 13:08:23 +0000
@@ -445,8 +445,12 @@
 }
 
 // RemoveRelation removes the relation with key from the topology.
-func (t *topology) RemoveRelation(key string) {
+func (t *topology) RemoveRelation(key string) error {
+	if err := t.assertRelation(key); err != nil {
+		return err
+	}
 	delete(t.topology.Relations, key)
+	return nil
 }
 
 // RelationsForService returns all relations that the service