Merge lp://staging/~niemeyer/juju-core/new-state-firewaller into lp://staging/~juju/juju-core/trunk

Reviewers: mp+125778_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/6548051/

Affected files:
   A [revision details]
   M juju/testing/conn.go
   M state/service.go
   M state/service_test.go
   M state/unit.go
   M state/unit_test.go
   M worker/firewaller/firewaller.go
   M worker/firewaller/firewaller_test.go

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.go#newcode283
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.go#newcode363
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.go#newcode368
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.go#newcode378
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.

https://codereview.appspot.com/6548051/

*** 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.go#newcode283
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.go#newcode363
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.go#newcode368
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.go#newcode378
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.

https://codereview.appspot.com/6548051/