state: add status to allwatcher unit and machine

state: add status to allwatcher unit and machine



https://code.launchpad.net/~rogpeppe/juju-core/274-megawatcher-unit-status/+merge/157765

Requires: https://code.launchpad.net/~rogpeppe/juju-core/275-multiwatcher-get/+merge/157764

(do not edit description out of merge proposal)

[rog, 2013-04-08 22:45:21]

Please take a look.

[rog, 2013-04-09 08:09:01]

Please take a look.

[rog, 2013-04-09 09:37:38]

Please take a look.

[TheMue, 2013-04-09 09:59:38]

LGTM, only one comment.

https://codereview.appspot.com/8534043/diff/5001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/8534043/diff/5001/state/megawatcher.go#newcode170
state/megawatcher.go:170: // We duplicate
Interesting comment. ;)

[rog, 2013-04-09 10:16:54]

Please take a look.

[rog, 2013-04-09 10:18:45]

Please take a look.

https://codereview.appspot.com/8534043/diff/5001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/8534043/diff/5001/state/megawatcher.go#newcode170
state/megawatcher.go:170: // We duplicate
On 2013/04/09 09:59:38, TheMue wrote:
> Interesting comment. ;)

indeed. done, another way.

[TheMue, 2013-04-09 10:20:33]

On 2013/04/09 10:18:45, rog wrote:
> Please take a look.
> 
> https://codereview.appspot.com/8534043/diff/5001/state/megawatcher.go
> File state/megawatcher.go (right):
> 
>
https://codereview.appspot.com/8534043/diff/5001/state/megawatcher.go#newcode170
> state/megawatcher.go:170: // We duplicate
> On 2013/04/09 09:59:38, TheMue wrote:
> > Interesting comment. ;)
> 
> indeed. done, another way.

Yes, looks fine now, it hasn't been needed.

[fwereade, 2013-04-09 10:31:25]

I understand your discomfort with the status round-trips, but I think we'll be
able to make it more sophisticated without excessive pain when we need to, so
LGTM.

https://codereview.appspot.com/8534043/diff/8002/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/8534043/diff/8002/state/apiserver/api_test.go#...
state/apiserver/api_test.go:1448: c.Logf("got:")
Hey, nice.

[rog, 2013-04-09 10:34:46]

*** Submitted:

state: add status to allwatcher unit and machine

R=TheMue, fwereade
CC=
https://codereview.appspot.com/8534043

https://codereview.appspot.com/8534043/diff/8002/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/8534043/diff/8002/state/apiserver/api_test.go#...
state/apiserver/api_test.go:1448: c.Logf("got:")
On 2013/04/09 10:31:25, fwereade wrote:
> Hey, nice.

i'd rather have a DeepEquals that, on failure, actually showed all the stuff
that it was comparing.

[frankban, 2013-04-09 10:37:58]

FWIW LGTM too!
Thanks Roger, we are now unblocked on the GUI side re: statuses.

https://codereview.appspot.com/8534043/diff/9001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/8534043/diff/9001/state/megawatcher.go#newcode285
state/megawatcher.go:285: // modify a primary entity.
only to modify

https://codereview.appspot.com/8534043/diff/9001/state/megawatcher_internal_t...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/8534043/diff/9001/state/megawatcher_internal_t...
state/megawatcher_internal_test.go:668: // StateWatcher tests the integration of
the state watcher
Maybe this is TestStateWatcher?

[rog, 2013-04-09 10:43:48]

https://codereview.appspot.com/8534043/diff/9001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/8534043/diff/9001/state/megawatcher.go#newcode285
state/megawatcher.go:285: // modify a primary entity.
On 2013/04/09 10:37:59, frankban wrote:
> only to modify

yup. will fix in subsequent branch.

https://codereview.appspot.com/8534043/diff/9001/state/megawatcher_internal_t...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/8534043/diff/9001/state/megawatcher_internal_t...
state/megawatcher_internal_test.go:668: // StateWatcher tests the integration of
the state watcher
On 2013/04/09 10:37:59, frankban wrote:
> Maybe this is TestStateWatcher?

indeed, good catch. will fix in subsequent branch.