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

One more little round.

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")
On 2012/07/18 07:52:44, TheMue wrote:
> On 2012/07/17 16:13:50, niemeyer wrote:
> > We can't panic here. stop can fail in pretty usual circumstances
that are
> > outside of the application's control (e.g. network issues). This
kind of error
> > condition must be properly managed as usual.

> Changed into logging and keeping the machine tracker, so that a future
removing
> won't fail or at least the finish of the firewaller will again try to
stop it.

It will not try to stop it again, as far as I can see. You got a Removed
notice for the respective node, and it won't show up again in the
future. There's also no good reason for a machineTracker to fail to
stop, unless there's a real problem. Unless you have something else in
mind, I think this should be a real error rather than a logged message.

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode143
worker/firewaller/firewaller.go:143: if !ok {
On 2012/07/18 07:52:44, TheMue wrote:
> In case of ok == false I need to send &machineUnitsChange{m, nil}.
Doing that
> explicitely I would need the same select as regular above. This way
change is
> nil, it would be send and aferwards the loop is left.

Ok, a comment sounds fine.

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#newcode11
worker/firewaller/firewaller.go:11: // Firewaller watches the state for
ports open or closed
Sorry, my typo: s/open/opened/

https://codereview.appspot.com/6374069/diff/12001/worker/firewaller/firewaller.go#newcode139
worker/firewaller/firewaller.go:139: case mt.changes <-
&machineUnitsChange{mt, change}:
What happens if mt.changes is closed?

We have to keep that kind of concern in mind when working with channels.

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