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

Gustavo Niemeyer (niemeyer) wrote :

LGTM with the following handled:
File worker/firewaller/firewaller.go (right):
worker/firewaller/firewaller.go:21: func NewFirewaller(st *state.State)
(*Firewaller, error) {
Nice to see how this was simplified by moving logic out.
worker/firewaller/firewaller.go:33: defer fw.finish()
Nice cleanup as well!
worker/firewaller/firewaller.go:51: log.Debugf("firewaller:
remove-machine %v", removedMachine.Id())
Either this should be dropped, or clarified to state what's being
reported. No machines are being removed here. I suggest just dropping.
worker/firewaller/firewaller.go:56: log.Debugf("firewaller: add-machine
worker/firewaller/firewaller.go:118: case <-mt.firewaller.tomb.Dying():
Why do we need this here? If the firewaller dies, it will stop the
worker/firewaller/firewaller.go:126: case <-mt.firewaller.tomb.Dying():

« Back to merge proposal