Code review comment for lp:~themue/juju-core/go-worker-firewaller-machines

mostly LGTM apart from the watcher stopping below,
and the unsafe testing access.

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode42
worker/firewaller/firewaller.go:42: // Get environment first.
defer watcher.Stop(fw.machineUnitsChanges, &fw.tomb)

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode43
worker/firewaller/firewaller.go:43: fw.environWatcher =
fw.st.WatchEnvironConfig()
defer watcher.Stop(fw.environWatcher, &fw.tomb)

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode54
worker/firewaller/firewaller.go:54: return
this isn't killing the machines watcher

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode69
worker/firewaller/firewaller.go:69: return
this isn't killing the environs watcher

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller.go#newcode90
worker/firewaller/firewaller.go:90: // TODO(mue) fill with life.
i like the image :-)

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller_test.go
File worker/firewaller/firewaller_test.go (right):

https://codereview.appspot.com/6374069/diff/1/worker/firewaller/firewaller_test.go#newcode100
worker/firewaller/firewaller_test.go:100: allMachines :=
fw.AllMachines()
i liked this initially, but after a moment's thought, i'm not sure about
it - it accesses fw.machines in a non-thread-safe way and it makes the
test slower than it should be.

i'll have a chat online about this - i have an idea :-)

https://codereview.appspot.com/6374069/