upgrader: new WatchForEnvironConfigChanges

upgrader: new WatchForEnvironConfigChanges

This is a NotifyWatcher which matches more what we
want to expose in the API

https://code.launchpad.net/~jameinel/juju-core/upgrader-api-watcher/+merge/172977

(do not edit description out of merge proposal)

[jameinel, 2013-07-04 09:10:33]

Please take a look.

[jameinel, 2013-07-04 11:07:01]

Please take a look.

[gz, 2013-07-04 11:09:02]

LGTM

https://codereview.appspot.com/10891044/diff/1/state/apiserver/upgrader/upgra...
File state/apiserver/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10891044/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader_test.go:85: wc.AssertOneChange()
Much nicer. This might also fall under William's desire to have the initial
event automatically consumed, in which case it would need to switch this assert
to AssertNoChange.

[fwereade, 2013-07-04 12:51:03]

LGTM modulo question below

https://codereview.appspot.com/10891044/diff/1/state/apiserver/upgrader/upgra...
File state/apiserver/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10891044/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader_test.go:83: defer statetesting.AssertStop(c,
w)
Can we depend on the ResourceRegistry cleanup for this? If not, should we be
able to?

https://codereview.appspot.com/10891044/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10891044/diff/1/state/state_test.go#newcode1108
state/state_test.go:1108: }
cfg, err = cfg.Apply(map[string]interface{"agent-version": vers.String()})

?

https://codereview.appspot.com/10891044/diff/4001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10891044/diff/4001/state/watcher.go#newcode1001
state/watcher.go:1001: // Watch return a NotifyWatcher waiting for the Environ
Config to change. This
s/Watch/WatchForEnvironConfigChanges/

[jameinel, 2013-07-04 14:13:24]

Please take a look.

https://codereview.appspot.com/10891044/diff/1/state/apiserver/upgrader/upgra...
File state/apiserver/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10891044/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader_test.go:83: defer statetesting.AssertStop(c,
w)
On 2013/07/04 12:51:03, fwereade wrote:
> Can we depend on the ResourceRegistry cleanup for this? If not, should we be
> able to?

There is a StopAll call in teardown, though it intentionally ignores errors
(during cleanup time).

This AssertStop is asserting that things get shut down cleanly.

I like having both. The TearDown ensures test cases are isolated, the AssertStop
asserts that this particular thing stops cleanly after the test is done using
it.

https://codereview.appspot.com/10891044/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader_test.go:85: wc.AssertOneChange()
On 2013/07/04 11:09:02, gz wrote:
> Much nicer. This might also fall under William's desire to have the initial
> event automatically consumed, in which case it would need to switch this
assert
> to AssertNoChange.

When and if NotifyWatcherC changes that, this will certainly be a place we need
to fix it. :)

https://codereview.appspot.com/10891044/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/10891044/diff/1/state/state_test.go#newcode1108
state/state_test.go:1108: }
On 2013/07/04 12:51:03, fwereade wrote:
> cfg, err = cfg.Apply(map[string]interface{"agent-version": vers.String()})
> 
> ?

I was copying it from elsewhere, if we have cfg.Apply that makes this code much
simpler, thanks.

I updated environs/jujutest/livetests.go as well.

Though having written this a couple times, we might want to put it somewhere
more general. state/testing perhaps?