uniter: skip hooks for implicit relations

uniter: skip hooks for implicit relations

see lp:1098444

https://code.launchpad.net/~fwereade/juju-core/uniter-implicit-relations/+merge/142867

(do not edit description out of merge proposal)

[fwereade, 2013-01-11 10:59:09]

Please take a look.

[rog, 2013-01-11 11:36:26]

looks fine, just a few comments and queries below.

https://codereview.appspot.com/7092044/diff/1/state/endpoint.go
File state/endpoint.go (right):

https://codereview.appspot.com/7092044/diff/1/state/endpoint.go#newcode94
state/endpoint.go:94: ep.Interface == "juju-info" &&
out of interest, can we have a relation named "juju-info" that *doesn't* have an
interface of kind "juju-info" ?

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer.go
File worker/uniter/relationer.go (right):

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer.go#new...
worker/uniter/relationer.go:135: } else if hi.Kind == hook.RelationBroken {
no need for an else clause here.

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer_test.go
File worker/uniter/relationer_test.go (left):

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:335: // be destroyed. Can't see a clean way to
do so currently.
this has been done?

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer_test.go
File worker/uniter/relationer_test.go (right):

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:428: checkNoHooks()
could you just check for no hooks at the end of the test with a simple
select-with-default, thus saving 150ms of test time?

it's true the diagnostics for failure won't be so good, but maybe that's ok.

[fwereade, 2013-01-11 13:00:46]

Please take a look.

https://codereview.appspot.com/7092044/diff/1/state/endpoint.go
File state/endpoint.go (right):

https://codereview.appspot.com/7092044/diff/1/state/endpoint.go#newcode94
state/endpoint.go:94: ep.Interface == "juju-info" &&
On 2013/01/11 11:36:26, rog wrote:
> out of interest, can we have a relation named "juju-info" that *doesn't* have
an
> interface of kind "juju-info" ?

We could, although that would probably indicate severe confusion on the part of
the charm author.

I don't think the answer to that question would change the code here, anyway: I
think it's sensible to demand that the endpoint has exactly the properties
implied by the single implicit relation that currently exists :).

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer.go
File worker/uniter/relationer.go (right):

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer.go#new...
worker/uniter/relationer.go:135: } else if hi.Kind == hook.RelationBroken {
On 2013/01/11 11:36:26, rog wrote:
> no need for an else clause here.

Done.

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer_test.go
File worker/uniter/relationer_test.go (left):

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:335: // be destroyed. Can't see a clean way to
do so currently.
On 2013/01/11 11:36:26, rog wrote:
> this has been done?

Not relevant here: the LeaveScope happens on SetDying for implicit relations,
and on relation-broken-commit for explicit ones; both tested elsewhere.

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer_test.go
File worker/uniter/relationer_test.go (right):

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:428: checkNoHooks()
On 2013/01/11 11:36:26, rog wrote:
> could you just check for no hooks at the end of the test with a simple
> select-with-default, thus saving 150ms of test time?
> 
> it's true the diagnostics for failure won't be so good, but maybe that's ok.

I want there to be a short pause to verify the join didn't cause a hook, because
it won't necessarily happen immediately; but after that, a single
select-with-default at the end SGTM.

[TheMue, 2013-01-11 13:01:35]

Mostly LGTM, see comments.

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer.go
File worker/uniter/relationer.go (right):

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer.go#new...
worker/uniter/relationer.go:135: } else if hi.Kind == hook.RelationBroken {
+1

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer_test.go
File worker/uniter/relationer_test.go (right):

https://codereview.appspot.com/7092044/diff/1/worker/uniter/relationer_test.g...
worker/uniter/relationer_test.go:428: checkNoHooks()
On 2013/01/11 11:36:26, rog wrote:
> could you just check for no hooks at the end of the test with a simple
> select-with-default, thus saving 150ms of test time?
> 
> it's true the diagnostics for failure won't be so good, but maybe that's ok.

Not sure if those 150ms are worth it. Using checkNoHooks() after each following
testing block makes it more explicit for a different maintainer.

[rog, 2013-01-14 17:00:07]

LGTM

[fwereade, 2013-01-14 21:28:20]

*** Submitted:

uniter: skip hooks for implicit relations

see lp:1098444

R=rog, TheMue
CC=
https://codereview.appspot.com/7092044