state: added watcher for service units

state: added watcher for service units

The added watcher returns informations about the
assigned or unassigned units of a service.

https://code.launchpad.net/~themue/juju-core/go-state-service-units-watcher/+merge/112054

(do not edit description out of merge proposal)

[TheMue, 2012-06-26 10:23:58]

Please take a look.

[fwereade, 2012-06-26 15:02:33]

LTM modulo below.

https://codereview.appspot.com/6325062/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6325062/diff/1/state/watcher.go#newcode52
state/watcher.go:52: Added, Deleted []*Unit
Just a suggestion: Added/Removed, to fit with service unit terminology
elsewhere.

https://codereview.appspot.com/6325062/diff/1/state/watcher.go#newcode79
state/watcher.go:79: // are assigned to or unassigned from the service. The
Added field in
added/removed? Units are assigned to machines, not services.

https://codereview.appspot.com/6325062/diff/1/state/watcher.go#newcode145
state/watcher.go:145: if unit == nil {
I don't think this is logically possible given the rest of the method; but I
guess we may as well check anyway.

https://codereview.appspot.com/6325062/diff/1/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6325062/diff/1/state/watcher_test.go#newcode127
state/watcher_test.go:127: {
I'm starting to feel that table-based tests which pass around shared state so
they can make use of pointer equality are more pain than they're worth, but YMM
legitimately V :).

https://codereview.appspot.com/6325062/diff/1/state/watcher_test.go#newcode180
state/watcher_test.go:180: case got, _ := <-unitsWatcher.Changes():
s/, _//

[TheMue, 2012-06-26 15:41:05]

Please take a look.

https://codereview.appspot.com/6325062/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6325062/diff/1/state/watcher.go#newcode52
state/watcher.go:52: Added, Deleted []*Unit
On 2012/06/26 15:02:33, fwereade wrote:
> Just a suggestion: Added/Removed, to fit with service unit terminology
> elsewhere.

Done.

https://codereview.appspot.com/6325062/diff/1/state/watcher.go#newcode79
state/watcher.go:79: // are assigned to or unassigned from the service. The
Added field in
On 2012/06/26 15:02:33, fwereade wrote:
> added/removed? Units are assigned to machines, not services.

Done.

https://codereview.appspot.com/6325062/diff/1/state/watcher.go#newcode145
state/watcher.go:145: if unit == nil {
On 2012/06/26 15:02:33, fwereade wrote:
> I don't think this is logically possible given the rest of the method; but I
> guess we may as well check anyway.

Done.

https://codereview.appspot.com/6325062/diff/1/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6325062/diff/1/state/watcher_test.go#newcode127
state/watcher_test.go:127: {
On 2012/06/26 15:02:33, fwereade wrote:
> I'm starting to feel that table-based tests which pass around shared state so
> they can make use of pointer equality are more pain than they're worth, but
YMM
> legitimately V :).

OK, here they are indeed a bit bloated, but typically they simplify scenarios
like this. In this case the wanted data just needs dynamic informations of the
data created in the tests.

[niemeyer, 2012-06-27 06:26:38]

Looking nice. A few details to sort out:

https://codereview.appspot.com/6325062/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6325062/diff/1/state/watcher.go#newcode50
state/watcher.go:50: // services.
This comment may be reflowed to make better use of screen space.

https://codereview.appspot.com/6325062/diff/1/state/watcher.go#newcode52
state/watcher.go:52: Added, Deleted []*Unit
On 2012/06/26 15:02:33, fwereade wrote:
> Just a suggestion: Added/Removed, to fit with service unit terminology
> elsewhere.

Every other watcher uses Deleted, though. We should pick one  side of it, and
move over. Please move on with Removed, Frank, but I've filed this for a
brain-break moment:

https://bugs.launchpad.net/juju-core/+bug/1018256

https://codereview.appspot.com/6325062/diff/5001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6325062/diff/5001/state/watcher.go#newcode56
state/watcher.go:56: // a service.
This needs to be more specific regarding what kind of change it's talking about.
It's not really observing changes *to the units*.

https://codereview.appspot.com/6325062/diff/5001/state/watcher.go#newcode81
state/watcher.go:81: // by Service.AllUnits().
s/()//

https://codereview.appspot.com/6325062/diff/5001/state/watcher.go#newcode152
state/watcher.go:152: return
There's probably no reason to return here. This will break the whole system
rather than just that one unit that for some weird reason disappeared. I suggest
logging and continuing.

https://codereview.appspot.com/6325062/diff/5001/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6325062/diff/5001/state/watcher_test.go#newcod...
state/watcher_test.go:120: func(s *state.Service, us []*state.Unit) error {
This can be put in a much simpler form that avoids those functions entirely. The
table should provide the script, but the fiddling may be done by the test
itself. For example:

var serviceUnitTests = []struct{
    op string
    n int
}{
    {"none", 0},
    {"add", 0},
    {"add", 1},
    {"remove", 0},
{

[TheMue, 2012-06-27 13:01:18]

Please take a look.

https://codereview.appspot.com/6325062/diff/5001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6325062/diff/5001/state/watcher.go#newcode56
state/watcher.go:56: // a service.
On 2012/06/27 06:26:38, niemeyer wrote:
> This needs to be more specific regarding what kind of change it's talking
about.
> It's not really observing changes *to the units*.

Done.

https://codereview.appspot.com/6325062/diff/5001/state/watcher.go#newcode81
state/watcher.go:81: // by Service.AllUnits().
On 2012/06/27 06:26:38, niemeyer wrote:
> s/()//

Done.

https://codereview.appspot.com/6325062/diff/5001/state/watcher.go#newcode152
state/watcher.go:152: return
On 2012/06/27 06:26:38, niemeyer wrote:
> There's probably no reason to return here. This will break the whole system
> rather than just that one unit that for some weird reason disappeared. I
suggest
> logging and continuing.

Done.

https://codereview.appspot.com/6325062/diff/5001/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6325062/diff/5001/state/watcher_test.go#newcod...
state/watcher_test.go:120: func(s *state.Service, us []*state.Unit) error {
On 2012/06/27 06:26:38, niemeyer wrote:
> This can be put in a much simpler form that avoids those functions entirely.
The
> table should provide the script, but the fiddling may be done by the test
> itself. For example:
> 
> var serviceUnitTests = []struct{
>     op string
>     n int
> }{
>     {"none", 0},
>     {"add", 0},
>     {"add", 1},
>     {"remove", 0},
> {

Done. Definitely looks better now, thx.

[niemeyer, 2012-06-28 07:01:35]

Thanks. LGTM, with a couple of trivials:

https://codereview.appspot.com/6325062/diff/11001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6325062/diff/11001/state/watcher.go#newcode55
state/watcher.go:55: // ServiceUnitsWatcher observes the adding and removing
addition and removal

https://codereview.appspot.com/6325062/diff/11001/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6325062/diff/11001/state/watcher_test.go#newco...
state/watcher_test.go:147: want = &state.ServiceUnitsChange{nil,
[]*state.Unit{units[test.idx]}}
units[test.idx] = nil

[TheMue, 2012-06-28 13:35:27]

*** Submitted:

state: added watcher for service units

The added watcher returns informations about the
assigned or unassigned units of a service.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6325062

https://codereview.appspot.com/6325062/diff/11001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6325062/diff/11001/state/watcher.go#newcode55
state/watcher.go:55: // ServiceUnitsWatcher observes the adding and removing
On 2012/06/28 07:01:35, niemeyer wrote:
> addition and removal

Done.

https://codereview.appspot.com/6325062/diff/11001/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6325062/diff/11001/state/watcher_test.go#newco...
state/watcher_test.go:147: want = &state.ServiceUnitsChange{nil,
[]*state.Unit{units[test.idx]}}
On 2012/06/28 07:01:35, niemeyer wrote:
> units[test.idx] = nil

Done.