state/api: AddCharm implemented

state/api: AddCharm implemented

This compliments the AddLocalCharm landed previously,
by supporting charm store (non-local really) charms.
AddCharm is a bit simpler than AddLocalCharm in that
it does not need to do a prepare + upload + update,
because the charm store is trusted to provide unique
revisions.

https://code.launchpad.net/~dimitern/juju-core/226-addcharm-api/+merge/199478

(do not edit description out of merge proposal)

[dimitern, 2013-12-18 15:29:27]

Please take a look.

[dimitern, 2013-12-18 16:19:24]

Please take a look.

[natefinch, 2013-12-18 17:22:07]

few small tweaks

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:865: // if it does not exists yet. Local charms
are not supported, only
s/exists/exist

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:890: store, ok := storeRepo.(interface {
Can you not just add Info to the repository interface?  That's all this is
really doing, right? That's a lot better than doing this.

Or just calculate the sha256 yourself.

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:912: size, err := archive.Seek(0, 2)
can you not just use os.Stat for this?  Seems like no need to open it and all
this.

[rog, 2013-12-18 18:35:38]

Looks really good in general. A few suggestions below.

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

https://codereview.appspot.com/43860043/diff/20001/state/api/client.go#newcod...
state/api/client.go:448: if curl == nil {
As usual, I don't think we need to test whether arguments are nil.
The runtime will panic and I think that's ok.

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:864: // AddCharm adds the given fully-defined
charm URL to the environment,
This comment has information in it that should be in the api client doc comment.

BTW a "fully defined" charm URL isn't well defined AFAICS.

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:890: store, ok := storeRepo.(interface {
On 2013/12/18 17:22:08, nate.finch wrote:
> Can you not just add Info to the repository interface?  That's all this is
> really doing, right? That's a lot better than doing this.
> 
> Or just calculate the sha256 yourself.

Ironically the CharmStore.Get method has already acquired and verified the
sha256 by this point, but we can't get that information.

We can't add Info to the Repository interface LocalRepository must also satisfy
that interface, and you can't know a charm's hash until it's bundled.

How about adding a new method to charm.CharmStore, say:

func (s *CharmStore) GetBundle(curl *URL) (bundle *Bundle, digest string, err
error)

and exporting a size method on charm.Bundle:

func (bundle *Bundle) Size() int64 {
    return bundle.size
}

Then the existing CharmStore GetBundle method can be changed to call GetBundle
(simply discarding the digest return), and we can assert the store to
bundleGetter:

type bundleGetter interface {
   GetBundle(curl *charm.URL) (bundle *Bundle, digest string, err error)
}

Then we won't need to scan the data yet again for the digest, you won't need to
stat or seek, and we only need to type-assert once.

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:912: size, err := archive.Seek(0, 2)
On 2013/12/18 17:22:08, nate.finch wrote:
> can you not just use os.Stat for this?  Seems like no need to open it and all
> this.

+1, but we'll need to open it regardless, and see above for another suggestion.

[dimitern, 2013-12-19 08:47:34]

Please take a look.

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

https://codereview.appspot.com/43860043/diff/20001/state/api/client.go#newcod...
state/api/client.go:448: if curl == nil {
On 2013/12/18 18:35:39, rog wrote:
> As usual, I don't think we need to test whether arguments are nil.
> The runtime will panic and I think that's ok.

Done.

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:864: // AddCharm adds the given fully-defined
charm URL to the environment,
On 2013/12/18 18:35:39, rog wrote:
> This comment has information in it that should be in the api client doc
comment.
> 
> BTW a "fully defined" charm URL isn't well defined AFAICS.

Added the doc comment to the client method and edited it a bit.

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:865: // if it does not exists yet. Local charms
are not supported, only
On 2013/12/18 17:22:08, nate.finch wrote:
> s/exists/exist

Done.

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:890: store, ok := storeRepo.(interface {
On 2013/12/18 18:35:39, rog wrote:
> On 2013/12/18 17:22:08, nate.finch wrote:
> > Can you not just add Info to the repository interface?  That's all this is
> > really doing, right? That's a lot better than doing this.
> > 
> > Or just calculate the sha256 yourself.
> 
> Ironically the CharmStore.Get method has already acquired and verified the
> sha256 by this point, but we can't get that information.
> 
> We can't add Info to the Repository interface LocalRepository must also
satisfy
> that interface, and you can't know a charm's hash until it's bundled.
> 
> How about adding a new method to charm.CharmStore, say:
> 
> func (s *CharmStore) GetBundle(curl *URL) (bundle *Bundle, digest string, err
> error)
> 
> and exporting a size method on charm.Bundle:
> 
> func (bundle *Bundle) Size() int64 {
>     return bundle.size
> }
> 
> Then the existing CharmStore GetBundle method can be changed to call GetBundle
> (simply discarding the digest return), and we can assert the store to
> bundleGetter:
> 
> type bundleGetter interface {
>    GetBundle(curl *charm.URL) (bundle *Bundle, digest string, err error)
> }
> 
> Then we won't need to scan the data yet again for the digest, you won't need
to
> stat or seek, and we only need to type-assert once.

I'd prefer not to mess with charm store code, and calculate the hash myself, as
it's least intrusive.

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:890: store, ok := storeRepo.(interface {
On 2013/12/18 17:22:08, nate.finch wrote:
> Can you not just add Info to the repository interface?  That's all this is
> really doing, right? That's a lot better than doing this.
> 
> Or just calculate the sha256 yourself.

Calculating the hash myself.

https://codereview.appspot.com/43860043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:912: size, err := archive.Seek(0, 2)
On 2013/12/18 17:22:08, nate.finch wrote:
> can you not just use os.Stat for this?  Seems like no need to open it and all
> this.

This is now changed, I still use Seek, bu just to rewind the file after getting
the hash.

[rog, 2013-12-19 09:10:22]

LGTM modulo the below. I don't think we should mind about making changes to the
charm package (the change I had in mind is about 5 lines)

https://codereview.appspot.com/43860043/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/43860043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:875: if charmURL.Schema == "local" {
Given that we're using CharmStore below, this should probably be :

if charmURL.Schema != "cs" {
    return fmt.Errorf("AddCharm only supports charmstore charms")
}

https://codereview.appspot.com/43860043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:899: archive, err :=
os.Open(downloadedCharm.(*charm.Bundle).Path)
This type assertion should probably be checked rather than panicking.

https://codereview.appspot.com/43860043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:905: size, err := io.Copy(hash, archive)
For the record, this hash is already calculated (and used) in the charm package
- there's no need to spread the logic around or to iterate through the  contents
once more. I don't think this adds to the generality of the logic - it just
makes things a little more complex and a little more inefficient.

[dimitern, 2013-12-19 09:58:05]

Please take a look.

https://codereview.appspot.com/43860043/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/43860043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:875: if charmURL.Schema == "local" {
On 2013/12/19 09:10:22, rog wrote:
> Given that we're using CharmStore below, this should probably be :
> 
> if charmURL.Schema != "cs" {
>     return fmt.Errorf("AddCharm only supports charmstore charms")
> }

Done.

https://codereview.appspot.com/43860043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:899: archive, err :=
os.Open(downloadedCharm.(*charm.Bundle).Path)
On 2013/12/19 09:10:22, rog wrote:
> This type assertion should probably be checked rather than panicking.

Done, although there's no way this can happen currently.

https://codereview.appspot.com/43860043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client.go:905: size, err := io.Copy(hash, archive)
On 2013/12/19 09:10:22, rog wrote:
> For the record, this hash is already calculated (and used) in the charm
package
> - there's no need to spread the logic around or to iterate through the 
contents
> once more. I don't think this adds to the generality of the logic - it just
> makes things a little more complex and a little more inefficient.

Noted, we can change it later, if needed.