state/api/upgrader: Implement WatchAPIVersion

state/api/upgrader: Implement WatchAPIVersion

This lets us create a client side params.NotifyWatcher in response to
Upgrader.WatchAPIVersion.

To make the testing sane, I exposed a way to trigger Sync in the API
Server (running in the dummy environment). These are the same Sync
calls that NotifyWatcherC was doing, except it needs to be done on a
different State object, which is not easily accessible to testing code.

https://code.launchpad.net/~jameinel/juju-core/upgrader-api-client-watcher/+merge/173511

(do not edit description out of merge proposal)

[jameinel, 2013-07-08 13:51:39]

Please take a look.

[jameinel, 2013-07-08 13:53:15]

Please take a look.

[jameinel, 2013-07-09 12:48:27]

Please take a look.

[fwereade, 2013-07-10 08:55:56]

For correctness, I think we need direct access to the backing state, not just
the arms-length sync method.

https://codereview.appspot.com/10890047/diff/5001/state/api/upgrader/upgrader...
File state/api/upgrader/upgrader_test.go (left):

https://codereview.appspot.com/10890047/diff/5001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader_test.go:18: coretesting
"launchpad.net/juju-core/testing"
A thought... would our lives be a little easier if we pulled the various testing
module imports into their own block?

https://codereview.appspot.com/10890047/diff/5001/state/api/upgrader/upgrader...
File state/api/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10890047/diff/5001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader_test.go:164: wc := statetesting.NewNotifyWatcherC(c,
s.State, w)
Hmm. I think we really ought to be constructing this with the *state.State used
for the API server... otherwise it's not really testing what it looks like it is
(can't control the sync/startsync that actually affects the watcher).

https://codereview.appspot.com/10890047/diff/5001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader_test.go:170: ver := version.Current.Number
This is a bit action-at-a-distance, because it's not 100% clear where
agent-version is originally set to version.Current.Number. I'll leave it to your
judgment though.

https://codereview.appspot.com/10890047/diff/5001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/10890047/diff/5001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader.go:51: err = nil
Crikey, was that me? Sorry.

https://codereview.appspot.com/10890047/diff/5001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader_test.go (left):

https://codereview.appspot.com/10890047/diff/5001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader_test.go:83: defer statetesting.AssertStop(c,
w)
I think this is still worth having, isn't it?

[jameinel, 2013-07-10 10:47:05]

Please take a look.

https://codereview.appspot.com/10890047/diff/5001/state/api/upgrader/upgrader...
File state/api/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10890047/diff/5001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader_test.go:164: wc := statetesting.NewNotifyWatcherC(c,
s.State, w)
On 2013/07/10 08:55:56, fwereade wrote:
> Hmm. I think we really ought to be constructing this with the *state.State
used
> for the API server... otherwise it's not really testing what it looks like it
is
> (can't control the sync/startsync that actually affects the watcher).

Done.

https://codereview.appspot.com/10890047/diff/5001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader_test.go:170: ver := version.Current.Number
On 2013/07/10 08:55:56, fwereade wrote:
> This is a bit action-at-a-distance, because it's not 100% clear where
> agent-version is originally set to version.Current.Number. I'll leave it to
your
> judgment though.

I changed it to just set an arbitrary version and then assert that we see that
event, but setting it again doesn't generate a new event.

https://codereview.appspot.com/10890047/diff/5001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/10890047/diff/5001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader.go:51: err = nil
On 2013/07/10 08:55:56, fwereade wrote:
> Crikey, was that me? Sorry.

Yeah, we didn't have a test for it, so we didn't notice. We had a test that
Results was correct, but we forgot to test that Error was nil.

https://codereview.appspot.com/10890047/diff/5001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader_test.go (left):

https://codereview.appspot.com/10890047/diff/5001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader_test.go:83: defer statetesting.AssertStop(c,
w)
On 2013/07/10 08:55:56, fwereade wrote:
> I think this is still worth having, isn't it?

We do clean up all resources in TearDown, but I'm fine leaving it in. I thought
it would be unhappy with multiple calls, but it is fine.

[fwereade, 2013-07-10 11:11:01]

LGTM, nice.

[dimitern, 2013-07-10 13:30:02]

LGTM

https://codereview.appspot.com/10890047/diff/11001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/10890047/diff/11001/juju/testing/conn.go#newco...
juju/testing/conn.go:220: type GetStater interface {
GetStateInAPIServer-er :D

[jameinel, 2013-07-10 17:26:13]

Please take a look.