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

Roger Peppe (rogpeppe) wrote :
File worker/firewaller/firewaller.go (right):
worker/firewaller/firewaller.go:44: defer close(machineUnitsChanges)
i think this line should go before the machinesWatcher stop, otherwise
we'll see eof on the channel before the firewaller has properly stopped.
maybe that doesn't matter; i'm not sure.
worker/firewaller/firewaller.go:52: if err != nil {
On 2012/07/17 16:13:50, niemeyer wrote:
> I believe we want to execute the line below no matter what. The err is
nil, the
> MustErr is supposed to explode pointing out the inconsistency.

in fact, if there's EOF from the machinesWatcher, it implies that the
machines watcher has already stopped, so there's no need to call Stop.
worker/firewaller/firewaller.go:118: changes: changes,
we could store this in firewaller to make it obvious that the changes
channel is shared between all machines.

« Back to merge proposal