state/api/upgrader: Implement client side

state/api/upgrader: Implement client side

This fixes some of the server side code, where Upgrader wasn't actually exposed
in the API, and a workaround for a JSON marshalling bug (not handling embedded
objects).

It implements Upgrader.SetTools and Upgrader.Tools in the client. The only
bit left to expose is the WatchAPIVersion code, which is going to be a bit
trickier.

I wanted to start getting feedback on this early, so here it is. I think this
can be landed without the rest of Upgrader API (though the rest needs to land
before we can switch the Worker over).

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

(do not edit description out of merge proposal)

[jameinel, 2013-07-03 13:07:30]

Please take a look.

[jameinel, 2013-07-03 13:10:11]

https://codereview.appspot.com/10711044/diff/1/state/apiserver/upgrader/upgra...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/10711044/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:124: logger.Debugf("Returning AgentTools:
%#v", agentTools)
We probably don't need this debug print if anyone wants me to pull it out.

[fwereade, 2013-07-03 13:58:43]

LGTM

https://codereview.appspot.com/10711044/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/10711044/diff/1/state/api/params/params.go#new...
state/api/params/params.go:254: AgentTools AgentTools
I think there's a "go1.1" string somewhere we're flagging this sort of thing
with?

https://codereview.appspot.com/10711044/diff/1/state/apiserver/upgrader/upgra...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/10711044/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:124: logger.Debugf("Returning AgentTools:
%#v", agentTools)
On 2013/07/03 13:10:11, jameinel wrote:
> We probably don't need this debug print if anyone wants me to pull it out.

+1

[john2, 2013-07-03 15:01:24]

The go1.1 flag isnt relevant because Im pretty sure this format nests the
values in a sub object. And we wont want to change the bytes on the wire in
a go 1.1 upgrade.

John
=:->
On Jul 3, 2013 5:58 PM, <fwereade@gmail.com> wrote:

> LGTM
>
>
> https://codereview.appspot.**com/10711044/diff/1/state/api/**
>
params/params.go<https://codereview.appspot.com/10711044/diff/1/state/api/params/params.go>
> File state/api/params/params.go (right):
>
> https://codereview.appspot.**com/10711044/diff/1/state/api/**
>
params/params.go#newcode254<https://codereview.appspot.com/10711044/diff/1/state/api/params/params.go#newcode254>
> state/api/params/params.go:**254: AgentTools AgentTools
> I think there's a "go1.1" string somewhere we're flagging this sort of
> thing with?
>
> https://codereview.appspot.**com/10711044/diff/1/state/**
>
apiserver/upgrader/upgrader.go<https://codereview.appspot.com/10711044/diff/1/state/apiserver/upgrader/upgrader.go>
> File state/apiserver/upgrader/**upgrader.go (right):
>
> https://codereview.appspot.**com/10711044/diff/1/state/**
>
apiserver/upgrader/upgrader.**go#newcode124<https://codereview.appspot.com/10711044/diff/1/state/apiserver/upgrader/upgrader.go#newcode124>
> state/apiserver/upgrader/**upgrader.go:124: logger.Debugf("Returning
> AgentTools: %#v", agentTools)
> On 2013/07/03 13:10:11, jameinel wrote:
>
>> We probably don't need this debug print if anyone wants me to pull it
>>
> out.
>
> +1
>
>
https://codereview.appspot.**com/10711044/<https://codereview.appspot.com/107...
>

[gz, 2013-07-03 16:07:32]

LGTM

https://codereview.appspot.com/10711044/diff/1/state/api/upgrader/upgrader.go
File state/api/upgrader/upgrader.go (right):

https://codereview.appspot.com/10711044/diff/1/state/api/upgrader/upgrader.go...
state/api/upgrader/upgrader.go:38: // TODO: Error case
Does this want a fmt.Errorf at least and a comment about testing it?

[mue, 2013-07-03 17:00:53]

LGTM

https://codereview.appspot.com/10711044/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/10711044/diff/1/state/api/params/params.go#new...
state/api/params/params.go:254: AgentTools AgentTools
On 2013/07/03 13:58:43, fwereade wrote:
> I think there's a "go1.1" string somewhere we're flagging this sort of thing
> with?

It's

// +build go1.1

But it works for the whole file.

[jameinel, 2013-07-04 07:53:17]

Please take a look.

https://codereview.appspot.com/10711044/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/10711044/diff/1/state/api/params/params.go#new...
state/api/params/params.go:254: AgentTools AgentTools
William was talking about using the TODO comment:

// TODO go1.1: Nest this when we can rely on go-1.1

However, because this is officially exposed in the bytes on the wire, we can't
actually do it even when we transition to 1.1.

Specifically if you have embedded types encoded by JSON it would look like:

{"Tag": "foobar", "Major": 1, "Minor": 2, ..., "Error": null}

However, because we have to work around it in go 1.0.3 we have:

{"AgentTools": {"Tag": "foobar", "Major": 1, ...}, "Error": null}

So while I do prefer the former syntax, we can't get it in go 1.0.3, and we
don't want to change our bytes-on-the-wire when we upgrade to go 1.1.

https://codereview.appspot.com/10711044/diff/1/state/api/upgrader/upgrader.go
File state/api/upgrader/upgrader.go (right):

https://codereview.appspot.com/10711044/diff/1/state/api/upgrader/upgrader.go...
state/api/upgrader/upgrader.go:38: // TODO: Error case
On 2013/07/03 16:07:32, gz wrote:
> Does this want a fmt.Errorf at least and a comment about testing it?

Done.