Support 'add-relation' in API.

Support 'add-relation' in API.

Support 'add-relation' in API, for real this time.

https://code.launchpad.net/~teknico/juju-core/support-add-relation-in-go-env-2/+merge/156030

(do not edit description out of merge proposal)

[teknico, 2013-03-28 17:19:18]

Please take a look.

[rog, 2013-03-28 17:35:55]

very nice.
LGTM modulo the minor comments below.

https://codereview.appspot.com/8094045/diff/1/cmd/juju/addrelation.go
File cmd/juju/addrelation.go (right):

https://codereview.appspot.com/8094045/diff/1/cmd/juju/addrelation.go#newcode36
cmd/juju/addrelation.go:36: Endpoints: c.Endpoints,
nice

https://codereview.appspot.com/8094045/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/8094045/diff/1/state/api/apiclient.go#newcode95
state/api/apiclient.go:95: func (c *Client) AddRelation(endpoint0, endpoint1
string) (*params.AddRelationResults, error) {
although currently we only allow a relation between two services, i think we've
been trying to keep things open for more in the future, so i think i'd have:
AddRelation(endpoints ...string)

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

https://codereview.appspot.com/8094045/diff/1/state/api/params/params.go#newc...
state/api/params/params.go:17: // AddRelationResults holds results of the
AddRelation call. The Endpoints
s/results of the/the results of a/

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

https://codereview.appspot.com/8094045/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:1363: c.Assert(res.Endpoints["wordpress"].Name,
Equals, "db")
or c.Assert(res.Endpoints, DeepEquals, map[string]charm.Relation {
    "wordpress": {
       ...
     },
     ...
}

?

https://codereview.appspot.com/8094045/diff/1/state/statecmd/addrelation.go
File state/statecmd/addrelation.go (right):

https://codereview.appspot.com/8094045/diff/1/state/statecmd/addrelation.go#n...
state/statecmd/addrelation.go:15: if len(args.Endpoints) != 2 {
i think we can lose this error message - InferEndpoints will pick it up.

https://codereview.appspot.com/8094045/diff/1/state/statecmd/addrelation_test.go
File state/statecmd/addrelation_test.go (right):

https://codereview.appspot.com/8094045/diff/1/state/statecmd/addrelation_test...
state/statecmd/addrelation_test.go:33:
c.Assert(endpoints.Endpoints["wordpress"].Name, Equals, "db")
c.Assert(endpoint.Endpoints, DeepEquals, ...) ?

https://codereview.appspot.com/8094045/diff/1/state/statecmd/addrelation_test...
state/statecmd/addrelation_test.go:56: c.Assert(res.Endpoints["mysql"].Name,
Equals, "server")
perhaps factor out the code in this test that's identical to the previous one?
(you could have a function that you call twice, the second time with the
arguments swapped)

[teknico, 2013-03-28 17:58:41]

Thanks for the review, fixed some things, added a few questions.

https://codereview.appspot.com/8094045/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/8094045/diff/1/state/api/apiclient.go#newcode95
state/api/apiclient.go:95: func (c *Client) AddRelation(endpoint0, endpoint1
string) (*params.AddRelationResults, error) {
rog wrote:
> although currently we only allow a relation between two
> services, i think we've been trying to keep things open
> for more in the future, so i think i'd have:
> AddRelation(endpoints ...string)

So you want to keep things open to relations among more than two services. Sure,
I'm very much into polyamory myself too! ;-)

https://codereview.appspot.com/8094045/diff/1/state/api/apiclient.go#newcode108
state/api/apiclient.go:108: func (c *Client) DestroyRelation(endpoint0,
endpoint1 string) error {
...and I guess I'll change DestroyRelation too while I'm at it.

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

https://codereview.appspot.com/8094045/diff/1/state/api/params/params.go#newc...
state/api/params/params.go:17: // AddRelationResults holds results of the
AddRelation call. The Endpoints
rog wrote:
> s/results of the/the results of a/

Done.

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

https://codereview.appspot.com/8094045/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:1363: c.Assert(res.Endpoints["wordpress"].Name,
Equals, "db")
rog wrote:
> or c.Assert(res.Endpoints, DeepEquals, map[string]charm.Relation {
>     "wordpress": {
>        ...
>      },
>      ...
> }
> 
> ?

charm.Relation has other fields in addition to Name, Interface and scope, so it
would not succeed, would it? Is there another compact way of expressing these
checks as one?

https://codereview.appspot.com/8094045/diff/1/state/statecmd/addrelation.go
File state/statecmd/addrelation.go (right):

https://codereview.appspot.com/8094045/diff/1/state/statecmd/addrelation.go#n...
state/statecmd/addrelation.go:15: if len(args.Endpoints) != 2 {
rog wrote:
> i think we can lose this error message -
> InferEndpoints will pick it up.

Uhm, ok, I'll try it.

https://codereview.appspot.com/8094045/diff/1/state/statecmd/addrelation_test.go
File state/statecmd/addrelation_test.go (right):

https://codereview.appspot.com/8094045/diff/1/state/statecmd/addrelation_test...
state/statecmd/addrelation_test.go:33:
c.Assert(endpoints.Endpoints["wordpress"].Name, Equals, "db")
rog wrote:
> c.Assert(endpoint.Endpoints, DeepEquals, ...) ?

See above.

https://codereview.appspot.com/8094045/diff/1/state/statecmd/addrelation_test...
state/statecmd/addrelation_test.go:56: c.Assert(res.Endpoints["mysql"].Name,
Equals, "server")
rog wrote:
> perhaps factor out the code in this test that's
> identical to the previous one?

Perhaps, perhaps not. :-)

[rog, 2013-03-28 18:03:50]

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

https://codereview.appspot.com/8094045/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:1363: c.Assert(res.Endpoints["wordpress"].Name,
Equals, "db")
On 2013/03/28 17:58:41, teknico wrote:
> rog wrote:
> > or c.Assert(res.Endpoints, DeepEquals, map[string]charm.Relation {
> >     "wordpress": {
> >        ...
> >      },
> >      ...
> > }
> > 
> > ?
> 
> charm.Relation has other fields in addition to Name, Interface and scope, so
it
> would not succeed, would it? Is there another compact way of expressing these
> checks as one?

i'd just include the other fields anyway. other places do it, and it seems
reasonable that you're checking the whole thing.

[teknico, 2013-03-29 12:09:54]

Please take a look.

[teknico, 2013-03-29 12:13:17]

Implemented all rogpeppe's suggestions, all tests pass.

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

https://codereview.appspot.com/8094045/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:1363: c.Assert(res.Endpoints["wordpress"].Name,
Equals, "db")
rog wrote:
> i'd just include the other fields anyway. other places do it,
> and it seems reasonable that you're checking the whole thing.

Done, and implemented all other suggestions.

[fwereade, 2013-03-29 12:50:50]

I'm a bit iffy about dropping the 2-endpoints check in the add-relation
command... do the benefits of the more-detailed error messages outweigh the
roundtrip costs? I'm not actually sure myself, so I'll leave it to your
judgment, I just wanted to make sure you'd considered it.

Otherwise LGTM, just one extra test I'd like to see because I'm not sure offhand
what the error message will be and I'd like it to be sensible.

https://codereview.appspot.com/8094045/diff/10001/state/statecmd/destroyrelat...
File state/statecmd/destroyrelation_test.go (left):

https://codereview.appspot.com/8094045/diff/10001/state/statecmd/destroyrelat...
state/statecmd/destroyrelation_test.go:92: }
Please add one more test here for attempted destruction of a peer relation (use
the "riak" charm, and just pass the "riak:ring" endpoint). If the error message
is unhelpful, consider improving it in state.

[teknico, 2013-03-29 14:31:37]

Please take a look.

[teknico, 2013-03-29 14:33:23]

*** Submitted:

Support 'add-relation' in API.

Support 'add-relation' in API, for real this time.

R=rog, fwereade
CC=
https://codereview.appspot.com/8094045