state: only real relations can be added

state: only real relations can be added

I'm starting to think InferEndpoints was a mistake: possible relations for a
given service can change between InferEndpoints and AddRelation (or, for
that matter, between InferEndpoints and EndpointsRelation). So, I *think*,
we should probably make the InferEndpoints call internal. That's a followup,
anyway, even if we end up deciding it's a smart move; thoughts, anyone?

https://code.launchpad.net/~fwereade/juju-core/strict-add-relation/+merge/145169

(do not edit description out of merge proposal)

[fwereade, 2013-01-29 05:49:19]

Please take a look.

[fwereade, 2013-01-29 06:07:31]

On 2013/01/29 05:49:19, fwereade wrote:
> Please take a look.

To expand on the InferEndpoints stuff: doing as I suggest would also make the
process of adding a relation somewhat cheaper, because we wouldn't be getting
services (to infer endpoints), discarding them, and then getting them again (to
add the actual relation).

[TheMue, 2013-01-31 17:59:28]

LGTM

[dimitern, 2013-02-04 16:32:09]

LGTM, one question.

https://codereview.appspot.com/7227045/diff/2001/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/7227045/diff/2001/state/relation_test.go#newco...
state/relation_test.go:44: 
Why the check for pre-existing relations is not relevant anymore?

https://codereview.appspot.com/7227045/diff/2001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/7227045/diff/2001/state/service_test.go#newcod...
state/service_test.go:422: eps, err :=
s.State.InferEndpoints([]string{"wordpress", "mysql"})
Isn't the point removing InferEndpoints altogether? Or this is a interim step?

[fwereade, 2013-02-07 17:49:41]

*** Submitted:

state: only real relations can be added

I'm starting to think InferEndpoints was a mistake: possible relations for a
given service can change between InferEndpoints and AddRelation (or, for
that matter, between InferEndpoints and EndpointsRelation). So, I *think*,
we should probably make the InferEndpoints call internal. That's a followup,
anyway, even if we end up deciding it's a smart move; thoughts, anyone?

R=TheMue, dimitern
CC=
https://codereview.appspot.com/7227045

https://codereview.appspot.com/7227045/diff/2001/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/7227045/diff/2001/state/relation_test.go#newco...
state/relation_test.go:44: 
On 2013/02/04 16:32:09, dimitern wrote:
> Why the check for pre-existing relations is not relevant anymore?

Because it's already tested, better...

https://codereview.appspot.com/7227045/diff/2001/state/relation_test.go#newco...
state/relation_test.go:119: // Check we cannot re-add the same relation,
regardless of endpoint ordering.
...here.

https://codereview.appspot.com/7227045/diff/2001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/7227045/diff/2001/state/service_test.go#newcod...
state/service_test.go:422: eps, err :=
s.State.InferEndpoints([]string{"wordpress", "mysql"})
On 2013/02/04 16:32:09, dimitern wrote:
> Isn't the point removing InferEndpoints altogether? Or this is a interim step?

Yeah, nobody's argued it's a bad idea yet, so I'll go ahead and call it a bug.