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

Proposed by Frank Mueller
Status: Merged
Approved by: Gustavo Niemeyer
Approved revision: 165
Merged at revision: 207
Proposed branch: lp:~themue/pyjuju/go-state-remove-relation
Merge into: lp:pyjuju/go
Prerequisite: lp:~themue/pyjuju/go-state-add-relation
Diff against target: 131 lines (+71/-4)
5 files modified
state/export_test.go (+13/-3)
state/relation.go (+5/-0)
state/state.go (+13/-0)
state/state_test.go (+35/-0)
state/topology.go (+5/-1)
To merge this branch: bzr merge lp:~themue/pyjuju/go-state-remove-relation
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email:

Description of the change

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

To post a comment you must log in.
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :
File state/relation.go (right):
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.

Revision history for this message
Gustavo Niemeyer (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().


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

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.

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

<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
<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

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :
File state/relation.go (right):
state/relation.go:84: func (r *ServiceRelation) Relation() *Relation {
This needs a test.
File state/state_test.go (right):
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.
File state/topology.go (right):
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

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

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

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

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

Unfortunately the CL is dirty again with unrelated changes.

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

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

LGTM, thanks. Just a trivial before submitting:
File state/state.go (right):
state/state.go:377: // TODO: Improve high-level errors, no low-level
Please move this to within the function. Doesn't make sense to have it
exposed in the API documentation.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/export_test.go'
2--- state/export_test.go 2012-05-28 23:15:30 +0000
3+++ state/export_test.go 2012-06-06 13:08:23 +0000
4@@ -1,6 +1,3 @@
7-// Copyright (c) 2011-2012 Canonical Ltd.
8 package state
10 import (
11@@ -27,4 +24,17 @@
12 return readConfigNode(st.zk, path)
13 }
15+// HasRelation tests if the topology contains a relation.
16+func HasRelation(st *State, relation *Relation) (bool, error) {
17+ t, err := readTopology(st.zk)
18+ if err != nil {
19+ return false, err
20+ }
21+ _, err = t.Relation(relation.key)
22+ if err != nil {
23+ return false, err
24+ }
25+ return true, nil
28 func Diff(a, b []string) []string { return diff(a, b) }
30=== modified file 'state/relation.go'
31--- state/relation.go 2012-05-31 16:19:15 +0000
32+++ state/relation.go 2012-06-06 13:08:23 +0000
33@@ -79,3 +79,8 @@
34 func (r *ServiceRelation) RelationName() string {
35 return r.relationName
36 }
38+// Relation returns the relation for this service relation.
39+func (r *ServiceRelation) Relation() *Relation {
40+ return &Relation{, r.relationKey}
43=== modified file 'state/state.go'
44--- state/state.go 2012-06-05 21:31:50 +0000
45+++ state/state.go 2012-06-06 13:08:23 +0000
46@@ -372,3 +372,16 @@
47 }
48 return &Relation{s, relationKey}, serviceRelations, nil
49 }
51+// RemoveRelation removes the relation.
52+// TODO: Improve high-level errors, no low-level passing.
53+func (s *State) RemoveRelation(relation *Relation) error {
54+ removeRelation := func(t *topology) error {
55+ _, err := t.Relation(relation.key)
56+ if err != nil {
57+ return fmt.Errorf("can't remove relation: %v", err)
58+ }
59+ return t.RemoveRelation(relation.key)
60+ }
61+ return retryTopologyChange(s.zk, removeRelation)
64=== modified file 'state/state_test.go'
65--- state/state_test.go 2012-06-06 10:17:40 +0000
66+++ state/state_test.go 2012-06-06 13:08:23 +0000
67@@ -1085,7 +1085,24 @@
68 c.Assert(serviceRelations[0].RelationScope(), Equals, state.ScopeGlobal)
69 c.Assert(serviceRelations[0].RelationRole(), Equals, state.RolePeer)
70 c.Assert(serviceRelations[0].RelationName(), Equals, "cache")
73+func (s *StateSuite) TestServiceRelationRelation(c *C) {
74+ dummy := s.addDummyCharm(c)
75+"riak", dummy)
76+ riakep := state.RelationEndpoint{"riak", "ring", "cache", state.RolePeer, state.ScopeGlobal}
77+ relation, serviceRelations, err :=
78+ c.Assert(err, IsNil)
79+ c.Assert(relation, NotNil)
80+ c.Assert(serviceRelations, HasLen, 1)
81+ ok, err := state.HasRelation(, serviceRelations[0].Relation())
82+ c.Assert(err, IsNil)
83+ c.Assert(ok, Equals, true)
84+ // Negative test.
85+ err =
86+ c.Assert(err, IsNil)
87+ _, err = state.HasRelation(, serviceRelations[0].Relation())
88+ c.Assert(err, ErrorMatches, `relation "relation-0000000000" does not exist`)
89 }
91 func (s *StateSuite) TestAddRelationMissingService(c *C) {
92@@ -1169,3 +1186,21 @@
93 c.Assert(err, IsNil)
94 c.Assert(env.Map(), DeepEquals, map[string]interface{}{"type": "dummy", "name": "foo"})
95 }
97+func (s *StateSuite) TestRemoveRelation(c *C) {
98+ dummy := s.addDummyCharm(c)
99+"riak", dummy)
100+ riakep := state.RelationEndpoint{"riak", "blog", "cache", state.RolePeer, state.ScopeGlobal}
101+ relation, _, err :=
102+ c.Assert(err, IsNil)
103+ c.Assert(relation, NotNil)
104+ hasRelation, err := state.HasRelation(, relation)
105+ c.Assert(err, IsNil)
106+ c.Assert(hasRelation, Equals, true)
107+ err =
108+ hasRelation, err = state.HasRelation(, relation)
109+ c.Assert(hasRelation, Equals, false)
110+ c.Assert(err, ErrorMatches, `relation "relation-0000000000" does not exist`)
111+ err =
112+ c.Assert(err, ErrorMatches, `can't remove relation: relation "relation-0000000000" does not exist`)
115=== modified file 'state/topology.go'
116--- state/topology.go 2012-05-31 09:48:26 +0000
117+++ state/topology.go 2012-06-06 13:08:23 +0000
118@@ -445,8 +445,12 @@
119 }
121 // RemoveRelation removes the relation with key from the topology.
122-func (t *topology) RemoveRelation(key string) {
123+func (t *topology) RemoveRelation(key string) error {
124+ if err := t.assertRelation(key); err != nil {
125+ return err
126+ }
127 delete(t.topology.Relations, key)
128+ return nil
129 }
131 // RelationsForService returns all relations that the service


People subscribed via source and target branches