worker/firewaller: fix for new state

worker/firewaller: fix for new state



https://code.launchpad.net/~niemeyer/juju-core/new-state-firewaller/+merge/125778

(do not edit description out of merge proposal)

[niemeyer, 2012-09-21 17:50:57]

Please take a look.

[TheMue, 2012-09-21 17:57:00]

LGTM, shows again the nice effect of using MongoDB. Great decision.

[dave_cheney.net, 2012-09-21 17:57:36]

LGTM. Minor quibbles.

https://codereview.appspot.com/6548051/diff/2001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/6548051/diff/2001/state/service_test.go#newcode71
state/service_test.go:71: c.Assert(s.service.IsExposed(), Equals, false)
This is fine, and I encourage this terser form. Does this represent a new
standard for err Asserts ?

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:283: watcher:    machine.WatchPrincipalUnits(),
// FIXME THIS MUST WATCH *ALL* UNITS.
// TODO(niemeyer) is more traditional.

[rog, 2012-09-21 18:00:24]

LGTM modulo the below

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:283: watcher:    machine.WatchPrincipalUnits(),
// FIXME THIS MUST WATCH *ALL* UNITS.
i think this comment should contain the string "TODO" somewhere

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:363: change := unit.OpenedPorts()
i think it would be reasonable for state.OpenedPorts to return the ports in
sorted order (it does define SortPorts after all).

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:368: ports = append([]state.Port(nil),
change...)
do we need to do this? after all, OpenedPorts is already making a copy to return
it.

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:378: // portsChanged returns whether old and new
contain the same set ports.
s/set ports/set of ports/

but i think portsEqual would be easier to understand,
the comment would actually be correct, and the
test above would not need to be negated.

[niemeyer, 2012-09-21 18:36:44]

*** Submitted:

worker/firewaller: fix for new state

R=TheMue, dfc, rog
CC=
https://codereview.appspot.com/6548051

https://codereview.appspot.com/6548051/diff/2001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/6548051/diff/2001/state/service_test.go#newcode71
state/service_test.go:71: c.Assert(s.service.IsExposed(), Equals, false)
On 2012/09/21 17:57:36, dfc wrote:
> This is fine, and I encourage this terser form. Does this represent a new
> standard for err Asserts ?

I've seen this in a few places already. I also think it's fine in terse cases
like this.

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:283: watcher:    machine.WatchPrincipalUnits(),
// FIXME THIS MUST WATCH *ALL* UNITS.
On 2012/09/21 17:57:36, dfc wrote:
> // TODO(niemeyer) is more traditional.

As discussed, BUG/FIXME generally means something else than TODO. I've added the
(niemeyer) bit, though.

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:363: change := unit.OpenedPorts()
On 2012/09/21 18:00:24, rog wrote:
> i think it would be reasonable for state.OpenedPorts to return the ports in
> sorted order (it does define SortPorts after all).

Sounds good. Done.

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:368: ports = append([]state.Port(nil),
change...)
On 2012/09/21 18:00:24, rog wrote:
> do we need to do this? after all, OpenedPorts is already making a copy to
return
> it.

Without the copy the same data is used from two different goroutines.

https://codereview.appspot.com/6548051/diff/2001/worker/firewaller/firewaller...
worker/firewaller/firewaller.go:378: // portsChanged returns whether old and new
contain the same set ports.
On 2012/09/21 18:00:24, rog wrote:
> s/set ports/set of ports/
> 
> but i think portsEqual would be easier to understand,
> the comment would actually be correct, and the
> test above would not need to be negated.

Done.