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.
File worker/firewaller/firewaller.go (right):
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.
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.
worker/firewaller/firewaller.go:60: panic("trying to remove unmanaged
"trying to remove machine that wasn't added"
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.
worker/firewaller/firewaller.go:105: type machine struct {
worker/firewaller/firewaller.go:115: func newMachine(mst *state.Machine,
fw *Firewaller, changes chan *machineUnitsChange) *machine {

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.
worker/firewaller/firewaller.go:141: return
case <-m.tomb.Dying():
worker/firewaller/firewaller.go:143: if !ok {
Checking for ok after using the received change?
worker/firewaller/firewaller.go:151: // stop lets the machine tracker
stop working.
// stop stops the machine tracker.
worker/firewaller/firewaller.go:157: type service struct {
A whole type just to enclose a bool feels like overkill.
worker/firewaller/firewaller.go:161: type unit struct {
Is this a unit tracker too?

« Back to merge proposal