uniter: use entity watchers

uniter: use entity watchers

The watcher stubs are now unused, and have been completely removed. Uniter
tests now pass.

https://code.launchpad.net/~fwereade/juju-core/working-uniter-tests/+merge/126210

Requires: https://code.launchpad.net/~fwereade/juju-core/relation-units-without-presence/+merge/126111

(do not edit description out of merge proposal)

[fwereade, 2012-09-25 10:48:45]

Please take a look.

[rog, 2012-09-25 13:41:26]

LGTM with one possible thought below.

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:305: ch, force, err := service.Charm()
Just a minor thought, probably just worth ignoring:

This code seems moderately repetitive. I look at it and wonder if there's a
small function that can be factored out and used in similar places in the other
modes. Then perhaps the reasons for the subtle differences between the handling
of upgrade in different modes might become more apparent.

For instance, why do the other modes need to do ReadCharmURL and this doesn't?

[niemeyer, 2012-09-25 14:51:39]

Nice and sweet.

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/modes.go#newco...
worker/uniter/modes.go:305: ch, force, err := service.Charm()
On 2012/09/25 13:41:26, rog wrote:
> For instance, why do the other modes need to do ReadCharmURL and this doesn't?

Because it has the charm in context.

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:575: func() {
This seems misplaced.

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:584: defer func() {
This can go into step itself.

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:599: c.Logf("%#v", s)
defer func() {
    if ctx.uniter != nil {
        c.Assert(ctx.uniter.Stop(), IsNil)
    }
}()

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:729: case <-time.After(1 * time.Second):
s/1/5/

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:815: timeout := time.After(10 * time.Second)
s/10/5/

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:819: ctx.st.StartSync() // to detect presence
change
The comment is a bit vague. The code below isn't handling presence directly.

Can we please talk live about this?

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:871: timeout := time.After(10 * time.Second)
s/10/5/

[niemeyer, 2012-09-25 14:52:00]

LGTM with those, btw.

[fwereade, 2012-09-25 23:19:06]

*** Submitted:

uniter: use entity watchers

The watcher stubs are now unused, and have been completely removed. Uniter
tests now pass.

R=rog, niemeyer
CC=
https://codereview.appspot.com/6568046

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:575: func() {
On 2012/09/25 14:51:39, niemeyer wrote:
> This seems misplaced.

Yeah.

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:584: defer func() {
On 2012/09/25 14:51:39, niemeyer wrote:
> This can go into step itself.

Actually, I think it's a matter of ctx.run(c, t.steps)

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:599: c.Logf("%#v", s)
On 2012/09/25 14:51:39, niemeyer wrote:
> defer func() {
>     if ctx.uniter != nil {
>         c.Assert(ctx.uniter.Stop(), IsNil)
>     }
> }()

Not here actually, I think.

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:729: case <-time.After(1 * time.Second):
On 2012/09/25 14:51:39, niemeyer wrote:
> s/1/5/

Done.

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:815: timeout := time.After(10 * time.Second)
On 2012/09/25 14:51:39, niemeyer wrote:
> s/10/5/

Done.

https://codereview.appspot.com/6568046/diff/2001/worker/uniter/uniter_test.go...
worker/uniter/uniter_test.go:819: ctx.st.StartSync() // to detect presence
change
On 2012/09/25 14:51:39, niemeyer wrote:
> The comment is a bit vague. The code below isn't handling presence directly.
> 
> Can we please talk live about this?

Agreed live: Sync just before status check.