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

Gustavo Niemeyer (niemeyer) wrote :

This is going in a great direction. Some initial comments.

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#newcode11
worker/firewaller/firewaller.go:11: // Firewaller manages the opening
and closing of ports.
// Firewaller watches the state for ports open or closed
// and reflects those changes onto the backing environment.

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode52
worker/firewaller/firewaller.go:52: if err != nil {
I believe we want to execute the line below no matter what. The err is
nil, the MustErr is supposed to explode pointing out the inconsistency.

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode60
worker/firewaller/firewaller.go:60: panic("trying to remove unmanaged
machine")
"trying to remove machine that wasn't added"

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode63
worker/firewaller/firewaller.go:63: panic("can't stop machine tracker")
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.

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode105
worker/firewaller/firewaller.go:105: type machine struct {
s/machine/machineTracker/

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode115
worker/firewaller/firewaller.go:115: func newMachine(mst *state.Machine,
fw *Firewaller, changes chan *machineUnitsChange) *machine {
newMachineTracker

This is not creating a machine. Please fix docs accordingly.

Also, the channel should be qualified with <- to make it clear that this
is an output channel.

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode141
worker/firewaller/firewaller.go:141: return
case <-m.tomb.Dying():

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode143
worker/firewaller/firewaller.go:143: if !ok {
Checking for ok after using the received change?

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode151
worker/firewaller/firewaller.go:151: // stop lets the machine tracker
stop working.
// stop stops the machine tracker.

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode157
worker/firewaller/firewaller.go:157: type service struct {
A whole type just to enclose a bool feels like overkill.

https://codereview.appspot.com/6374069/diff/8001/worker/firewaller/firewaller.go#newcode161
worker/firewaller/firewaller.go:161: type unit struct {
Is this a unit tracker too?

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

« Back to merge proposal