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

Gustavo Niemeyer (niemeyer) wrote :

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

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode63
worker/firewaller/firewaller.go:63: panic("can't stop machine tracker")
As I mentioned on IRC, I think your approach of logging is actually
right. What's misguiding here is the comment: the machine tracker *is*
stopped if we got here. The message should look like:

"machine tracker returned error when stopping: %v"

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

https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewaller.go#newcode63
worker/firewaller/firewaller.go:63: delete(fw.machines,
removedMachine.Id())
This should also be moved above the if block.

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

« Back to merge proposal