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

Roger Peppe (rogpeppe) wrote :

mostly LGTM apart from the watcher stopping below,
and the unsafe testing access.
File worker/firewaller/firewaller.go (right):
worker/firewaller/firewaller.go:42: // Get environment first.
defer watcher.Stop(fw.machineUnitsChanges, &fw.tomb)
worker/firewaller/firewaller.go:43: fw.environWatcher =
defer watcher.Stop(fw.environWatcher, &fw.tomb)
worker/firewaller/firewaller.go:54: return
this isn't killing the machines watcher
worker/firewaller/firewaller.go:69: return
this isn't killing the environs watcher
worker/firewaller/firewaller.go:90: // TODO(mue) fill with life.
i like the image :-)
File worker/firewaller/firewaller_test.go (right):
worker/firewaller/firewaller_test.go:100: 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 :-)

« Back to merge proposal