api: AddLocalCharm method to upload charms via API

api: AddLocalCharm method to upload charms via API

This introduces api.Client.AddLocalCharm that takes
care of all the internals about uploading a local charm
through the API server. Because charm upload needs
authentication, caching of API client credentials
is introduced, so after Login succeeds the client
can upload charms without asking for credentials.
Also introduced a NotImplementedError, which is
returned by UploadCharm when the API server is
detected to not support charm uploads (needed for
1.16 compatibility later on).

Next, we'll use this instead of Conn.PutCharm in
deploy and upgrade-charm commands.

https://code.launchpad.net/~dimitern/juju-core/225-upload-charm-via-api/+merge/199251

(do not edit description out of merge proposal)

[dimitern, 2013-12-17 10:08:15]

Please take a look.

[rog, 2013-12-17 12:21:31]

LGTM modulo the below. Thanks!

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

https://codereview.appspot.com/43330043/diff/1/state/api/apiclient.go#newcode39
state/api/apiclient.go:39: // address holds the cached API server address we
used to login,
s/address/address (including the port)/

?

But I'm not sure about "address" here - it's more than the address; it's really
the url root. I'd either call this field something like that (e.g. urlBase) or
lose the https:// prefix.

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

https://codereview.appspot.com/43330043/diff/1/state/api/client.go#newcode362
state/api/client.go:362: type NotImplementedError struct {
I think we should probably be consistent with the rest of the API error handling
here and use an error code.

https://codereview.appspot.com/43330043/diff/1/state/api/client.go#newcode389
state/api/client.go:389: if archive, err = ioutil.TempFile("", "charm"); err !=
nil {
There's strictly speaking no need to create a temporary
file - we can stream the bundle directly to the POST
request. Worth considering, but perhaps it's good that
we get to check that the BundleTo succeeds before sending
it.

[rog, 2013-12-17 13:23:14]

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

https://codereview.appspot.com/43330043/diff/1/state/api/client.go#newcode420
state/api/client.go:420: resp, err := utils.GetNonValidatingHTTPClient().Do(req)
This seems wrong to me. Why aren't we validating the server?

https://codereview.appspot.com/43330043/diff/1/state/api/client.go#newcode428
state/api/client.go:428: }
Perhaps a comment saying that the response is in the same form regardless of the
http status code here?

[dimitern, 2013-12-17 15:20:09]

Please take a look.

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

https://codereview.appspot.com/43330043/diff/1/state/api/apiclient.go#newcode39
state/api/apiclient.go:39: // address holds the cached API server address we
used to login,
On 2013/12/17 12:21:32, rog wrote:
> s/address/address (including the port)/
> 
> ?
> 
> But I'm not sure about "address" here - it's more than the address; it's
really
> the url root. I'd either call this field something like that (e.g. urlBase) or
> lose the https:// prefix.

I'll call it serverRoot instead. The https:// prefix is needed, so I can easily
substitute it for http:// in tests and not have to spin up a whole TLS server.

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

https://codereview.appspot.com/43330043/diff/1/state/api/client.go#newcode362
state/api/client.go:362: type NotImplementedError struct {
On 2013/12/17 12:21:32, rog wrote:
> I think we should probably be consistent with the rest of the API error
handling
> here and use an error code.

Done.

https://codereview.appspot.com/43330043/diff/1/state/api/client.go#newcode389
state/api/client.go:389: if archive, err = ioutil.TempFile("", "charm"); err !=
nil {
On 2013/12/17 12:21:32, rog wrote:
> There's strictly speaking no need to create a temporary
> file - we can stream the bundle directly to the POST
> request. Worth considering, but perhaps it's good that
> we get to check that the BundleTo succeeds before sending
> it.

Yeah, my idea was the added check for BundleTo,

[dimitern, 2013-12-17 15:31:58]

Please take a look.

[rog, 2013-12-17 16:00:49]

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

https://codereview.appspot.com/43330043/diff/1/state/api/client.go#newcode362
state/api/client.go:362: type NotImplementedError struct {
On 2013/12/17 15:20:10, dimitern wrote:
> On 2013/12/17 12:21:32, rog wrote:
> > I think we should probably be consistent with the rest of the API error
> handling
> > here and use an error code.
> 
> Done.

Sorry, that wasn't quite what I had in mind.
api clients are currently using rpc.IsNoSuchRequest
to determine whether an api call is unimplemented.

I think it would be good if all api calls returned
the same kind of error when they're unimplemented,
otherwise we've got two ways of finding out if something
is unimplemented depending on the underlying implementation.

I can see at least two possibilities for doing that:

1)
return &rpc.RequestError{Message: `no such request "POST /charms"`}

2)
rpc.CodeNoSuchRequest = "no such request"
params.CodeNotImplemented = rpc.CodeNoSuchRequest
params.IsNotImplemented = errorcode == params.CodeNotImplemented

make the rpc package return error code CodeNoSuchRequest on method-not-defined
error (it'll need to do the prefix test too for compatibility)
change the various callers of rpc.IsNoSuchRequest to call
params.IsNotImplemented.

Obviously 1) is least effort, but has a slight ickiness factor to
it because we're not actually making an rpc request.

However I suggest we go with that for the time being and file a bug to change
things later - api clients should probably not be relying on
rpc-defined functions.