Code review comment for lp:~themue/juju-core/go-firewaller-added-global-mode

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Needs some love.

https://codereview.appspot.com/6713054/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6713054/diff/1/worker/firewaller/firewaller.go#newcode195
worker/firewaller/firewaller.go:195: m, err := machined.machine()
Why do we need the machine, and the instance? It's feeling somewhat
hackish. If it's in global mode, it should be handed off to a method
that deals with global mode *only* in some sensible point, rather than
interleaving unnecessary logic like that.

https://codereview.appspot.com/6713054/diff/1/worker/firewaller/firewaller.go#newcode252
worker/firewaller/firewaller.go:252: if fw.initialPorts[port] {
It's not clear to me how initialPorts is working. What if there are open
ports that should not be? What if there are initialPorts that were
closed and need to be re-opened?

The overall approach is asking for some tasteful consideration.

https://codereview.appspot.com/6713054/

« Back to merge proposal