Add destroy-relation to API.

Add destroy-relation to API.



https://code.launchpad.net/~bac/juju-core/remove-relation/+merge/153548

(do not edit description out of merge proposal)

[bac, 2013-03-15 17:42:36]

Please take a look.

[dimitern, 2013-03-15 18:02:32]

I like it in general, but I think in can be improved a bit more:

https://codereview.appspot.com/7610046/diff/1001/cmd/juju/destroyrelation.go
File cmd/juju/destroyrelation.go (right):

https://codereview.appspot.com/7610046/diff/1001/cmd/juju/destroyrelation.go#...
cmd/juju/destroyrelation.go:40: Endpoint0: c.Endpoints[3],
can you put a comment here please, explaining the indices?

https://codereview.appspot.com/7610046/diff/1001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/api/params/params.go#n...
state/api/params/params.go:12: Endpoint0 string
why not Endpoint []string instead? will save some typing at least, provided it's
handled correctly inside.

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/api_test.go
File state/apiserver/api_test.go (left):

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:408: c.Assert(err, IsNil)
excellent, thank you (for all the removed redundant asserts)!

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:1311: //err:       "no relation found",
why? it's exactly like above. it's immediately iffy..

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/apiserver.go...
state/apiserver/apiserver.go:366: // DestroyRelation destroys the relation
between the named endpoints.
they're not really "named", just given, I think.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
File state/statecmd/destroyrelation.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation.go:12: endpoints := []string{args.Endpoint0,
args.Endpoint1}
(see above) fixing the args type to []string will save you this.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
File state/statecmd/destroyrelation_test.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation_test.go:28: about:     "successfully destroy a
relation",
if the order really doesn't matter, I'd like to see a test with swapped
endpoints please.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation_test.go:58: Endpoint0: t.endpoints[0],
...this will also be simplified.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation_test.go:66: //svc.Refresh()
d?

[bac, 2013-03-15 19:58:54]

Thanks for the review Dimiter.

I have addressed your concerns but argue against changing the endpoints in the
params structure.  Please take another look.

https://codereview.appspot.com/7610046/diff/1001/cmd/juju/destroyrelation.go
File cmd/juju/destroyrelation.go (right):

https://codereview.appspot.com/7610046/diff/1001/cmd/juju/destroyrelation.go#...
cmd/juju/destroyrelation.go:40: Endpoint0: c.Endpoints[3],
This was a mistake that has been fixed.  I'd inadvertently not run these tests
so it wasn't caught.  The tests, as written, catch the mistake.

The first index should be 0, so I don't think any explanation is required.

https://codereview.appspot.com/7610046/diff/1001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/api/params/params.go#n...
state/api/params/params.go:12: Endpoint0 string
I think it is better to be explicit that exactly two strings are accepted and it
is more readable in the future.

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:1311: //err:       "no relation found",
This test relied on the results of the previous test.  (err was improperly
commented out.)  I don't like these types of test inter-dependencies and I'm
glad you questioned it.  I've now created a third service that can be used here
to demonstrate the failure seen when the endpoints don't have an established
relationship.

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/apiserver.go...
state/apiserver/apiserver.go:366: // DestroyRelation destroys the relation
between the named endpoints.
On 2013/03/15 18:02:32, dimitern wrote:
> they're not really "named", just given, I think.

Done.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
File state/statecmd/destroyrelation.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation.go:12: endpoints := []string{args.Endpoint0,
args.Endpoint1}
The command 'juju destroy-relation' requires exactly two endpoints and so does
the API call.  Other call sites of InferEndpoints can use one or two endpoints.

Since my use-case requires two it seems sensible to me to have exactly two and
force the check earlier than later.

I'm open to further discussion about it but that's my thinking.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
File state/statecmd/destroyrelation_test.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation_test.go:28: about:     "successfully destroy a
relation",
What you've requested is kind hard to do given the structure of the tests:  one
setup followed by data-driven tests.  I can't destroyrelation(a,b) and then
destroyrelation(b,a) because the relation is already gone.

I have added another test where there is a relation between wordpress and
logging but then destroyrelation call is (logging, wordpress).

I also changed the passing criteria to attempt to get the specified relationship
and demonstrating it is gone.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation_test.go:58: Endpoint0: t.endpoints[0],
As above.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation_test.go:66: //svc.Refresh()
On 2013/03/15 18:02:32, dimitern wrote:
> d?

Done.

[bac, 2013-03-15 20:19:42]

Please take a look.

[fwereade, 2013-03-15 23:40:49]

On 2013/03/15 20:19:42, bac wrote:
> Please take a look.

I think we need a bit more discussion re params, and I'm a little bit
uncomfortable with the testing; let me know your thoughts.

[fwereade, 2013-03-15 23:41:15]

Blast, thought I pressed "m" above. Sorry.

https://codereview.appspot.com/7610046/diff/1001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/api/params/params.go#n...
state/api/params/params.go:12: Endpoint0 string
On 2013/03/15 19:58:54, bac wrote:
> I think it is better to be explicit that exactly two strings are accepted and
it
> is more readable in the future.
> 

I'd much prefer "Requirer" and "Provider" in that order if we go this way, but I
argue elsewhere for []string and would favour that solution.

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:1311: //err:       "no relation found",
On 2013/03/15 19:58:54, bac wrote:
> This test relied on the results of the previous test.  (err was improperly
> commented out.)  I don't like these types of test inter-dependencies and I'm
> glad you questioned it.  I've now created a third service that can be used
here
> to demonstrate the failure seen when the endpoints don't have an established
> relationship.

For the record, I have become firmly -1 on the use of table-based tests that
depend on poor insulation between rows; I have done it a number of times myself,
but I feel it's usually turned out badly.

I suspect that if you did just write this test without a table it'd end up
rather more readable, though.

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/apiserver.go...
state/apiserver/apiserver.go:366: // DestroyRelation destroys the relation
between the named endpoints.
On 2013/03/15 19:58:54, bac wrote:
> On 2013/03/15 18:02:32, dimitern wrote:
> > they're not really "named", just given, I think.
> 
> Done.

Sorry, -1 on this -- IMO the endpoints actually are "named" not "given", because
an endpoint specifier looks like `mysql:server`, but the name `mysql` might be
used here and unambiguously interpreted in context as the endpoint
`mysql:server`.

I'll leave it to your judgment if this feels like pointless quibbling, but I'm
pretty sure there's another method sharing the same comment somewhere earlier in
the diff and those at least should be consistent.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
File state/statecmd/destroyrelation.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation.go:12: endpoints := []string{args.Endpoint0,
args.Endpoint1}
On 2013/03/15 19:58:54, bac wrote:
> The command 'juju destroy-relation' requires exactly two endpoints and so does
> the API call.  Other call sites of InferEndpoints can use one or two
endpoints.
> 
> Since my use-case requires two it seems sensible to me to have exactly two and
> force the check earlier than later.
> 
> I'm open to further discussion about it but that's my thinking.
> 

I *think* that this concern is not best addressed in this layer. The state api
was consciously designed with arbitrary endpoints in mind, and I would prefer to
maintain that property in the absence of a compelling argument for dropping it.

I'd prefer to say that the GUI and the CLI should have similar responsibilities
wrt turning user input into sensible args, and that the api package should
favour just passing the args through directly, and let the state package accept
or reject it using the existing error messages (which I hope are reasonably
helpful and clear regarding the specific problems encountered).

Sane?

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
File state/statecmd/destroyrelation_test.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation_test.go:28: about:     "successfully destroy a
relation",
On 2013/03/15 19:58:54, bac wrote:
> What you've requested is kind hard to do given the structure of the tests: 
one
> setup followed by data-driven tests.  I can't destroyrelation(a,b) and then
> destroyrelation(b,a) because the relation is already gone.
> 
> I have added another test where there is a relation between wordpress and
> logging but then destroyrelation call is (logging, wordpress).
> 
> I also changed the passing criteria to attempt to get the specified
relationship
> and demonstrating it is gone.
> 

Again, my gut says that these tests might be expressed more clearly in a flatter
imperative style. Not a requirement, just a suggestion.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation_test.go:69: c.Assert(rels, HasLen, 0)
I think it's equivalent and simpler to capture rel in :52 and assert
`state.IsNotFound(rel.Refresh())`.

[dave_cheney.net, 2013-03-18 05:54:56]

https://codereview.appspot.com/7610046/diff/12001/state/api/apiserver.go.THIS
File state/api/apiserver.go.THIS (left):

https://codereview.appspot.com/7610046/diff/12001/state/api/apiserver.go.THIS...
state/api/apiserver.go.THIS:1: package api
Is this an accident, how did this get into the diff ?

https://codereview.appspot.com/7610046/diff/12001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/7610046/diff/12001/state/api/params/params.go#...
state/api/params/params.go:9: // DestroyRelation holds the parameters for making
the DestroyRelationcall.
space before call.

[bac, 2013-03-18 15:25:17]

Thanks for the excellent suggestions.  Will re-propose shortly.

https://codereview.appspot.com/7610046/diff/1001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/api/params/params.go#n...
state/api/params/params.go:12: Endpoint0 string
As Jesse Jackson would say, "The question is moot."

I've gone with []string

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/api_test.go#...
state/apiserver/api_test.go:1311: //err:       "no relation found",
Excellent news.  It was my belief that the project was quite proud of this
table-driven approach and reviewers in the past have strongly encouraged their
use.  I'm happy to redo these tests as independent cases.

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/apiserver/apiserver.go...
state/apiserver/apiserver.go:366: // DestroyRelation destroys the relation
between the named endpoints.
A variant of this comment appeared in three places.  I have changed them all to:

// DestroyRelation removes the relation between the specified endpoints.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
File state/statecmd/destroyrelation.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation.go:12: endpoints := []string{args.Endpoint0,
args.Endpoint1}
OK, I'll make the change to use []string.

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
File state/statecmd/destroyrelation_test.go (right):

https://codereview.appspot.com/7610046/diff/1001/state/statecmd/destroyrelati...
state/statecmd/destroyrelation_test.go:69: c.Assert(rels, HasLen, 0)
Much nicer, thanks.

https://codereview.appspot.com/7610046/diff/12001/state/api/apiserver.go.THIS
File state/api/apiserver.go.THIS (left):

https://codereview.appspot.com/7610046/diff/12001/state/api/apiserver.go.THIS...
state/api/apiserver.go.THIS:1: package api
On 2013/03/18 05:54:56, dfc wrote:
> Is this an accident, how did this get into the diff ?

It is the correction of an accident.  The file shown here had been accidentally
checked in previously by someone else and I am simply removing it.

https://codereview.appspot.com/7610046/diff/12001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/7610046/diff/12001/state/api/params/params.go#...
state/api/params/params.go:9: // DestroyRelation holds the parameters for making
the DestroyRelationcall.
On 2013/03/18 05:54:56, dfc wrote:
> space before call.

Done.

[bac, 2013-03-18 16:57:29]

Please take a look.

[dave_cheney.net, 2013-03-19 00:35:27]

LGTM with one minor request.

https://codereview.appspot.com/7610046/diff/20001/state/statecmd/destroyrelat...
File state/statecmd/destroyrelation_test.go (right):

https://codereview.appspot.com/7610046/diff/20001/state/statecmd/destroyrelat...
state/statecmd/destroyrelation_test.go:35: Endpoints: []string{"riak",
"wordpress"},
I would like to see a test where len(params.DestroyRelation.Endpoints) != 2

[dimitern, 2013-03-19 09:44:24]

Thanks, this is better - LGTM.

https://codereview.appspot.com/7610046/diff/20001/state/statecmd/destroyrelat...
File state/statecmd/destroyrelation_test.go (right):

https://codereview.appspot.com/7610046/diff/20001/state/statecmd/destroyrelat...
state/statecmd/destroyrelation_test.go:17: func (s *DestroyRelationSuite)
_setUpDestroyRelationScenario(c *C) {
no underscores please

https://codereview.appspot.com/7610046/diff/20001/state/statecmd/destroyrelat...
state/statecmd/destroyrelation_test.go:35: Endpoints: []string{"riak",
"wordpress"},
On 2013/03/19 00:35:27, dfc wrote:
> I would like to see a test where len(params.DestroyRelation.Endpoints) != 2

+1

[bac, 2013-03-19 13:31:21]

Thanks for the reviews.  Please have another look at what I've done wrt the
number of arguments.

https://codereview.appspot.com/7610046/diff/20001/state/statecmd/destroyrelat...
File state/statecmd/destroyrelation_test.go (right):

https://codereview.appspot.com/7610046/diff/20001/state/statecmd/destroyrelat...
state/statecmd/destroyrelation_test.go:17: func (s *DestroyRelationSuite)
_setUpDestroyRelationScenario(c *C) {
On 2013/03/19 09:44:24, dimitern wrote:
> no underscores please

Done.

https://codereview.appspot.com/7610046/diff/20001/state/statecmd/destroyrelat...
state/statecmd/destroyrelation_test.go:35: Endpoints: []string{"riak",
"wordpress"},
I've added the test.  In the process of doing so I discovered my understanding
of how InferEndpoints works is incorrect.  If there is only one endpoint then
the service must be a peer.  But then trying to destroy the peer relationship
results in the error:

"cannot destroy relation \"riak:ring\": is a peer relation"

Note that the command-line "juju destroy-relation" enforces that there must be
two services.  The API should do the same.  (Perhaps that is what William meant
in an earlier comment that I didn't understand.)

In this branch I have moved the check for two services into the common code in
statecmd.  I have added a test for making the call to the API with only one
service and show that it fails.  I believe this is the correct course of action.

[dimitern, 2013-03-19 13:36:54]

Thanks!

https://codereview.appspot.com/7610046/diff/20001/state/statecmd/destroyrelat...
File state/statecmd/destroyrelation_test.go (right):

https://codereview.appspot.com/7610046/diff/20001/state/statecmd/destroyrelat...
state/statecmd/destroyrelation_test.go:17: func (s *DestroyRelationSuite)
_setUpDestroyRelationScenario(c *C) {
On 2013/03/19 13:31:22, bac wrote:
> On 2013/03/19 09:44:24, dimitern wrote:
> > no underscores please
> 
> Done.

I can still see them, probably you didn't repropose?

https://codereview.appspot.com/7610046/diff/20001/state/statecmd/destroyrelat...
state/statecmd/destroyrelation_test.go:35: Endpoints: []string{"riak",
"wordpress"},
On 2013/03/19 13:31:22, bac wrote:
> I've added the test.  In the process of doing so I discovered my understanding
> of how InferEndpoints works is incorrect.  If there is only one endpoint then
> the service must be a peer.  But then trying to destroy the peer relationship
> results in the error:
> 
> "cannot destroy relation \"riak:ring\": is a peer relation"
> 
> Note that the command-line "juju destroy-relation" enforces that there must be
> two services.  The API should do the same.  (Perhaps that is what William
meant
> in an earlier comment that I didn't understand.)
> 
> In this branch I have moved the check for two services into the common code in
> statecmd.  I have added a test for making the call to the API with only one
> service and show that it fails.  I believe this is the correct course of
action.

Great, thanks! Indeed - you cannot remove a peer relation unless you destroy the
service itself as well. Peer relations are automatically created and joined.

[bac, 2013-03-19 13:52:04]

Please take a look.

[dimitern, 2013-03-19 14:03:01]

LGTM

[bac, 2013-03-19 14:06:58]

*** Submitted:

Add destroy-relation to API.

R=dimitern, fwereade, dfc
CC=
https://codereview.appspot.com/7610046