worker/addressupdater: wire up

worker/addressupdater: wire up

It could do with some more comprehensive
testing, but it might be OK to land anyway.

https://code.launchpad.net/~rogpeppe/juju-core/433-addressupdater-integrate/+merge/188949

(do not edit description out of merge proposal)

[rog, 2013-10-02 22:13:59]

Please take a look.

[gz, 2013-10-02 22:34:05]

LGTM, just some general scribbles as I read through.

https://codereview.appspot.com/14306043/diff/3001/juju/testing/utils.go
File juju/testing/utils.go (right):

https://codereview.appspot.com/14306043/diff/3001/juju/testing/utils.go#newco...
juju/testing/utils.go:17: func ChangeEnvironConfig(c *gc.C, st *state.State,
change func(coretesting.Attrs) coretesting.Attrs) {
I assume this gets cleaned up at test end anyway, so doesn't need any of its
own?

https://codereview.appspot.com/14306043/diff/3001/provider/dummy/environs.go
File provider/dummy/environs.go (right):

https://codereview.appspot.com/14306043/diff/3001/provider/dummy/environs.go#...
provider/dummy/environs.go:881: inst0.addresses = append(inst0.addresses[:0],
addrs...)
..must remember this idiom for replacing slice contents.

https://codereview.appspot.com/14306043/diff/3001/worker/addressupdater/obser...
File worker/addressupdater/observer.go (right):

https://codereview.appspot.com/14306043/diff/3001/worker/addressupdater/obser...
worker/addressupdater/observer.go:82: func (obs *environObserver) Environ()
environs.Environ {
Okay, so a getter with a lock does come out as a pretty simple way of keeping
the environ up to date.

https://codereview.appspot.com/14306043/diff/3001/worker/addressupdater/obser...
File worker/addressupdater/observer_test.go (right):

https://codereview.appspot.com/14306043/diff/3001/worker/addressupdater/obser...
worker/addressupdater/observer_test.go:32: func (s *observerSuite)
TestEnvironmentChanges(c *gc.C) {
This test is kind of giant, I guess there's not much value to splitting it in
two though as the starts-invalid and becomes-valid cases have so much overlap.

https://codereview.appspot.com/14306043/diff/3001/worker/addressupdater/updat...
File worker/addressupdater/updater_test.go (right):

https://codereview.appspot.com/14306043/diff/3001/worker/addressupdater/updat...
worker/addressupdater/updater_test.go:108: case
<-time.After(coretesting.ShortWait):
Okay, another reasonable use of manditory short wait in a test.

https://codereview.appspot.com/14306043/diff/3001/worker/addressupdater/worke...
File worker/addressupdater/worker.go (right):

https://codereview.appspot.com/14306043/diff/3001/worker/addressupdater/worke...
worker/addressupdater/worker.go:80: return insts[0].Addresses()
Really must remove the re-check logic in Addresses, give most uses will be like
this on a fresh Instance.

https://codereview.appspot.com/14306043/diff/3001/worker/addressupdater/worke...
File worker/addressupdater/worker_test.go (right):

https://codereview.appspot.com/14306043/diff/3001/worker/addressupdater/worke...
worker/addressupdater/worker_test.go:111: // Associate the odd-numbered machines
with an instance.
This is a little cute, but I guess some complexity in a integration-y test like
this is fine, and it's well commented even though the setup and assertions are
seperated in the file.

[rog, 2013-10-03 00:28:03]

Please take a look.