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

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/

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

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/

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

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/

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/

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/

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/