cmd/juju/get: Use the API

cmd/juju/get: Use the API

Change "juju get" to call api.Client().ServiceGet() instead of
statecmd.ServiceGet(). Then I moved all the tests and implementation
over to state/apiserver/client/get*

I actually kept the code out of client.go because as we move things,
that file will get *huge* if all API calls end up in one file. It also
meant that the test for get.go end up in get_test.go which is nice and
straightforward.

The tests are just moving the existing tests (and changing the suite)
except for one change which I want to highlight. ServiceGet over the API
is returning float64(0) for skill-level, when the direct state call was
returning int64(0). I'm guessing that YAML and BSON are preserving the
int64 type, while JSON is not. (It is also possible that it was just
preserving the content in memory and returning it again, and if it
actually did a full DB round trip it would have lost the type.) Anyway,
I'm guessing we aren't strict with types, otherwise we'd have to change
our JSON api to send type information for all entries in a
map[]interface{}. But I did want us to be clear that type changing was
happening.

https://code.launchpad.net/~jameinel/juju-core/cli-api-get/+merge/182175

(do not edit description out of merge proposal)

[jameinel, 2013-08-26 17:44:38]

Please take a look.

[rog, 2013-08-27 08:35:29]

LGTM modulo some thoughts below

https://codereview.appspot.com/12744052/diff/1/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/12744052/diff/1/juju/apiconn_test.go#newcode96
juju/apiconn_test.go:96: client.Close()
shouldn't this test be in state/api ?

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

https://codereview.appspot.com/12744052/diff/1/state/api/client.go#newcode237
state/api/client.go:237: // Close closes the underlying State connection
s/underlying/client's underlying/

https://codereview.appspot.com/12744052/diff/1/state/api/client.go#newcode238
state/api/client.go:238: // Client is unique amonge the api.State facades in
closing its own State
s/amonge/among/

https://codereview.appspot.com/12744052/diff/1/state/api/client.go#newcode239
state/api/client.go:239: // connection, but it is also the only object we want
to expose to the CLI.
I'm not sure that this logic is very strong.

Perhaps; "but it is conventional to use a Client object
without any access to its underlying state connection" ?

https://codereview.appspot.com/12744052/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/12744052/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:104: return serviceGet(c.api.state, args)
Why not just define Client.ServiceGet inside get.go?

https://codereview.appspot.com/12744052/diff/1/state/apiserver/client/get_tes...
File state/apiserver/client/get_test.go (right):

https://codereview.appspot.com/12744052/diff/1/state/apiserver/client/get_tes...
state/apiserver/client/get_test.go:128: "value": float64(0),
I'd like to see at least one test that checks what happens if you use a value
larger than 2^53.

[fwereade, 2013-08-27 10:15:02]

LGTM

https://codereview.appspot.com/12744052/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/12744052/diff/1/juju/api.go#newcode53
juju/api.go:53: // NewAPIClientFromName returns an APIConn pointing at the
environName
s/APIConn/api.Client/

https://codereview.appspot.com/12744052/diff/1/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/12744052/diff/1/juju/apiconn_test.go#newcode96
juju/apiconn_test.go:96: client.Close()
On 2013/08/27 08:35:29, rog wrote:
> shouldn't this test be in state/api ?

+1

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

https://codereview.appspot.com/12744052/diff/1/state/api/client.go#newcode239
state/api/client.go:239: // connection, but it is also the only object we want
to expose to the CLI.
On 2013/08/27 08:35:29, rog wrote:
> I'm not sure that this logic is very strong.
> 
> Perhaps; "but it is conventional to use a Client object
> without any access to its underlying state connection" ?

+1

https://codereview.appspot.com/12744052/diff/1/state/apiserver/client/get_tes...
File state/apiserver/client/get_test.go (right):

https://codereview.appspot.com/12744052/diff/1/state/apiserver/client/get_tes...
state/apiserver/client/get_test.go:128: "value": float64(0),
On 2013/08/27 08:35:29, rog wrote:
> I'd like to see at least one test that checks what happens if you use a value
> larger than 2^53.

Yeah, for confirmation that a bug exists, if nothing else.

[jameinel, 2013-08-28 07:12:26]

The actual proposal will end up in the other review request (with a prereq) but
I wanted to respond to your requests here.

https://codereview.appspot.com/12744052/diff/1/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/12744052/diff/1/juju/api.go#newcode53
juju/api.go:53: // NewAPIClientFromName returns an APIConn pointing at the
environName
On 2013/08/27 10:15:02, fwereade wrote:
> s/APIConn/api.Client/

done. I cleaned this up quite a bit to make it clearer what the api.Client
really was.

https://codereview.appspot.com/12744052/diff/1/juju/apiconn_test.go
File juju/apiconn_test.go (right):

https://codereview.appspot.com/12744052/diff/1/juju/apiconn_test.go#newcode96
juju/apiconn_test.go:96: client.Close()
On 2013/08/27 08:35:29, rog wrote:
> shouldn't this test be in state/api ?

This was originally here because there are no tests in state/api/

but I added some tests there because we really do want things that are testing
that level to live there. The tests in state/apiserver/client/ are sort of
testing both the client and the server, so they may or may not get moved.
Close() is definitely testing just this object, though.

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

https://codereview.appspot.com/12744052/diff/1/state/api/client.go#newcode239
state/api/client.go:239: // connection, but it is also the only object we want
to expose to the CLI.
On 2013/08/27 10:15:02, fwereade wrote:
> On 2013/08/27 08:35:29, rog wrote:
> > I'm not sure that this logic is very strong.
> > 
> > Perhaps; "but it is conventional to use a Client object
> > without any access to its underlying state connection" ?
> 
> +1

Done.

https://codereview.appspot.com/12744052/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/12744052/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:104: return serviceGet(c.api.state, args)
On 2013/08/27 08:35:29, rog wrote:
> Why not just define Client.ServiceGet inside get.go?

Because that would be the first method not defined inside this file. I
personally find it hard to find methods-on-structs when they can be distributed
across many files.

There was also the fact that it changes 'c.api.state' into 'st', but it turns
out there was just one reference anyway, so it didn't really matter.


I can do it if you felt it was important enough to comment on.

https://codereview.appspot.com/12744052/diff/1/state/apiserver/client/get_tes...
File state/apiserver/client/get_test.go (right):

https://codereview.appspot.com/12744052/diff/1/state/apiserver/client/get_tes...
state/apiserver/client/get_test.go:128: "value": float64(0),
On 2013/08/27 10:15:02, fwereade wrote:
> On 2013/08/27 08:35:29, rog wrote:
> > I'd like to see at least one test that checks what happens if you use a
value
> > larger than 2^53.
> 
> Yeah, for confirmation that a bug exists, if nothing else.

I added a test, but it revealed a specific problem.

charm.Config().ParseSettingStrings() does a lot of work to cast things to the
exact types as defined by the Option definitions.

If you look at the data definition it has the statement "type": "int".
And we have the charm/Option.parse(name, str) function which looks at "int" and
makes sure to strconv.ParseInt(str, 10, 64)

So over the API we now lose all that type specific work that is being done in
the API Server. It really does feel like we need to be running a pass over the
data once more on the client side and mapping str/int/float/boolean over to the
right types locally.

Thoughts?