Implemented the NeedsUpgradeWatcher for Units.

Implemented the NeedsUpgradeWatcher for Units.

It's based on the changed ContentWatcher. Also the tests have
been harmonized (better timeout handling).

https://code.launchpad.net/~themue/juju/go-state-unit-needs-upgrade-watcher/+merge/102465

(do not edit description out of merge proposal)

[TheMue, 2012-04-18 09:12:39]

Please take a look.

[niemeyer, 2012-04-18 19:25:34]

Looks nice. Here are a few comments to get you going tomorrow, but you'll
probably have to delete the merge proposal (sort the comments first!) and
resubmit, as the pre-req is missing. Have a look at the diffs.. it's piling up
all the changes from prior branches that you've done.

https://codereview.appspot.com/6050053/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6050053/diff/1/state/unit.go#newcode495
state/unit.go:495: // changes may be observed as a single event in the channel.
These comments need love. Please read through them, and also check out the
original comments in the CL you submitted originally.

https://codereview.appspot.com/6050053/diff/1/state/unit.go#newcode521
state/unit.go:521: case change := <-w.watcher.Changes():
As per the other review, I believe this needs an "ok" check to avoid panicking.

https://codereview.appspot.com/6050053/diff/1/state/watch_test.go
File state/watch_test.go (right):

https://codereview.appspot.com/6050053/diff/1/state/watch_test.go#newcode10
state/watch_test.go:10: receiveChange := func(w *state.ConfigWatcher)
(*state.ConfigNode, bool, bool) {
Please reorganize this as a table, as suggested in the other review.

https://codereview.appspot.com/6050053/diff/1/state/watch_test.go#newcode73
state/watch_test.go:73: receiveChange := func(w *state.ConfigWatcher)
(*state.ConfigNode, bool, bool) {
Ditto.

https://codereview.appspot.com/6050053/diff/1/state/watch_test.go#newcode102
state/watch_test.go:102: receiveChange := func(w *state.NeedsUpgradeWatcher)
(bool, bool, bool) {
Ditto.

[TheMue, 2012-04-19 15:25:20]

Please take a look.

https://codereview.appspot.com/6050053/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6050053/diff/1/state/unit.go#newcode495
state/unit.go:495: // changes may be observed as a single event in the channel.
On 2012/04/18 19:25:34, niemeyer wrote:
> These comments need love. Please read through them, and also check out the
> original comments in the CL you submitted originally.

Done.

https://codereview.appspot.com/6050053/diff/1/state/unit.go#newcode521
state/unit.go:521: case change := <-w.watcher.Changes():
On 2012/04/18 19:25:34, niemeyer wrote:
> As per the other review, I believe this needs an "ok" check to avoid
panicking.

Done.

https://codereview.appspot.com/6050053/diff/1/state/watch_test.go
File state/watch_test.go (right):

https://codereview.appspot.com/6050053/diff/1/state/watch_test.go#newcode10
state/watch_test.go:10: receiveChange := func(w *state.ConfigWatcher)
(*state.ConfigNode, bool, bool) {
On 2012/04/18 19:25:34, niemeyer wrote:
> Please reorganize this as a table, as suggested in the other review.

Done.

https://codereview.appspot.com/6050053/diff/1/state/watch_test.go#newcode73
state/watch_test.go:73: receiveChange := func(w *state.ConfigWatcher)
(*state.ConfigNode, bool, bool) {
On 2012/04/18 19:25:34, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/6050053/diff/1/state/watch_test.go#newcode102
state/watch_test.go:102: receiveChange := func(w *state.NeedsUpgradeWatcher)
(bool, bool, bool) {
On 2012/04/18 19:25:34, niemeyer wrote:
> Ditto.

Done.

[niemeyer, 2012-04-20 01:18:02]

LGTM with a couple of trivials. Thanks for bearing with me.

https://codereview.appspot.com/6050053/diff/4001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6050053/diff/4001/state/state.go#newcode296
state/state.go:296: // A nonexistent node is treated as an empty node.
s/non/non-/

https://codereview.appspot.com/6050053/diff/4001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6050053/diff/4001/state/unit.go#newcode495
state/unit.go:495: // a single event in the channel.
Let's please use something similar to what's in the watcher package:

// Changes returns a channel that will receive notifications
// about upgrades for the unit. Note that multiple changes
// may be observed as a single event in the channel.

[TheMue, 2012-04-20 09:51:08]

*** Submitted:

Implemented the NeedsUpgradeWatcher for Units.

It's based on the changed ContentWatcher. Also the tests have
been harmonized (better timeout handling).

R=
CC=
https://codereview.appspot.com/6050053

https://codereview.appspot.com/6050053/diff/4001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6050053/diff/4001/state/state.go#newcode296
state/state.go:296: // A nonexistent node is treated as an empty node.
On 2012/04/20 01:18:02, niemeyer wrote:
> s/non/non-/

Done. Funnily had both writings as review hints. Compared via Google, with dash
won 22 to 10 mio. *smile*

[gustavo.niemeyer, 2012-04-23 22:56:39]

https://codereview.appspot.com/6050053/diff/4001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6050053/diff/4001/state/state.go#newcode296
state/state.go:296: // A nonexistent node is treated as an empty node.
On 2012/04/20 09:51:09, TheMue wrote:
> On 2012/04/20 01:18:02, niemeyer wrote:
> > s/non/non-/
> 
> Done. Funnily had both writings as review hints. Compared via Google, with
dash
> won 22 to 10 mio. *smile*

This tends to be a reasonable source as well:

http://oxforddictionaries.com/definition/non-existent