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

Gustavo Niemeyer (niemeyer) wrote :

LGTM with the following handled:

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

https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewaller.go#newcode21
worker/firewaller/firewaller.go:21: func NewFirewaller(st *state.State)
(*Firewaller, error) {
Nice to see how this was simplified by moving logic out.

https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewaller.go#newcode33
worker/firewaller/firewaller.go:33: defer fw.finish()
Nice cleanup as well!

https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewaller.go#newcode51
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.

https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewaller.go#newcode56
worker/firewaller/firewaller.go:56: log.Debugf("firewaller: add-machine
%v", m.id)
Ditto.

https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewaller.go#newcode118
worker/firewaller/firewaller.go:118: case <-mt.firewaller.tomb.Dying():
Why do we need this here? If the firewaller dies, it will stop the
tracker.

https://codereview.appspot.com/6374069/diff/19001/worker/firewaller/firewaller.go#newcode126
worker/firewaller/firewaller.go:126: case <-mt.firewaller.tomb.Dying():
Ditto.

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

« Back to merge proposal