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

William Reade (fwereade) wrote :
File worker/firewaller/firewaller.go (right):
worker/firewaller/firewaller.go:16: environWatcher
Shouldn't be a field, only used inside loop().

Also, I think I mentioned this somewhere before... why do the firewaller
and the provisioner each hold independent instances of what should be
the same Environ? Given that we can (AIUI) change environ config, and
use the environ itself, safely, at any time in any goroutine, I don't
see the benefit of duplicating the
get-me-an-environ-and-keep-it-up-to-date logic.
worker/firewaller/firewaller.go:17: machinesWatcher
Shouldn't be a field, only used inside loop().
worker/firewaller/firewaller.go:42: // Get environment first.
On 2012/07/16 16:40:46, rog wrote:
> defer watcher.Stop(fw.machineUnitsChanges, &fw.tomb)

defer close(fw.machineUnitsChanges)
defer watcher.Stop(fw.machinesWatcher, &fw.tomb)

(although as stated above I don't think machinesWatcher needs to be a
worker/firewaller/firewaller.go:53: }

(except it shouln't be a field, and IMO shouldn't even exist here)
worker/firewaller/firewaller.go:68: }
watcher.MustErr, as above.
worker/firewaller/firewaller.go:75: // case of not managed machine?
Feels like a panic situation to me.
worker/firewaller/firewaller.go:86: fw.machines[addedMachine.Id()] = m
go machine.loop()

In fact, plausibly, m := newMachine(addedMachine)
worker/firewaller/firewaller.go:129: if !ok {

« Back to merge proposal