juju-metadata: add generate-tools subcommand

juju-metadata: add generate-tools subcommand

This is a new subcommand for the juju-metadata plugin,
which can be used to generate simplestreams metadata
(index and products) for an existing set of tools.

The command will list the tools in the public bucket
or a local directory, and generate the index and
tools metadata in the same location.

https://code.launchpad.net/~axwalk/juju-core/juju-metadata-generate-tools/+merge/183070

(do not edit description out of merge proposal)

[axw, 2013-08-30 03:58:24]

Please take a look.

[axw, 2013-08-30 05:04:34]

Please take a look.

[axw, 2013-08-30 05:46:37]

Please take a look.

[wallyworld, 2013-08-30 06:40:42]

Coming along nicely. But not quite there. See various comments.

https://codereview.appspot.com/13417043/diff/7001/cmd/plugins/juju-metadata/t...
File cmd/plugins/juju-metadata/toolsmetadata.go (right):

https://codereview.appspot.com/13417043/diff/7001/cmd/plugins/juju-metadata/t...
cmd/plugins/juju-metadata/toolsmetadata.go:30: const pathPrefix = "tools/"
I'd make this "releases"

https://codereview.appspot.com/13417043/diff/7001/cmd/plugins/juju-metadata/t...
cmd/plugins/juju-metadata/toolsmetadata.go:55: env, err :=
environs.NewFromName(c.EnvName)
There's little point using an env to read the tools - the tools are readily
accessible directly from "https://juju-dist.s3.amazonaws.com/" (see synctools).
Ideally, this tool would look also at the specified directory (via -d) and read
the tools tarballs from there (under releases) and generate the metadata. Or
even better, take a URL like file:// as an arg (but dir will be fine for the
first iteration of this).

The only reason an env would be used is so that tools metadata could be written
to the env's private bucket. However, what will ultimately happen is synctools
will evolve to not just copy the tools tarballs like it does now, but also write
the metadata using the new MarshallToolsMetadata method in tools. This command
will be used for dev oriented things like easily adding new tool tarballs into
the mix so they can used by juju.

https://codereview.appspot.com/13417043/diff/7001/cmd/plugins/juju-metadata/t...
cmd/plugins/juju-metadata/toolsmetadata.go:74: // it's from the public or
private storage at this point.
urlPath should have "releases" prepended.
If the base url is http://juju.canonical.com/tools, then tools tarballs will be
at 
releases/foo.tar.gz etc
and simplestreams metadata will be at
streams/v1/index.json etc

https://codereview.appspot.com/13417043/diff/7001/environs/emptystorage.go
File environs/emptystorage.go (right):

https://codereview.appspot.com/13417043/diff/7001/environs/emptystorage.go#ne...
environs/emptystorage.go:17: var EmptyStorage Storage = struct {
Do we still need this given the implementation has changed?

https://codereview.appspot.com/13417043/diff/7001/environs/localstorage/stora...
File environs/localstorage/storage.go (right):

https://codereview.appspot.com/13417043/diff/7001/environs/localstorage/stora...
environs/localstorage/storage.go:71: }
What's this used for?

https://codereview.appspot.com/13417043/diff/7001/environs/simplestreams/simp...
File environs/simplestreams/simplestreams.go (right):

https://codereview.appspot.com/13417043/diff/7001/environs/simplestreams/simp...
environs/simplestreams/simplestreams.go:179: func (c *ItemCollection)
UnmarshalJSON(b []byte) error {
Please move this and associated code/tests to json.go

https://codereview.appspot.com/13417043/diff/7001/environs/simplestreams/simp...
File environs/simplestreams/simplestreams_test.go (right):

https://codereview.appspot.com/13417043/diff/7001/environs/simplestreams/simp...
environs/simplestreams/simplestreams_test.go:439: "c": 123
Could we please make this a big number to test the issue is solved

https://codereview.appspot.com/13417043/diff/7001/environs/tools/simplestream...
File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/13417043/diff/7001/environs/tools/simplestream...
environs/tools/simplestreams.go:24: ProductMetadataPath =
"streams/v1/com.ubuntu.juju:released:tools.json"
This is only used when generating data, please move it as per the comment below
and possibly unexport.

https://codereview.appspot.com/13417043/diff/7001/environs/tools/simplestream...
environs/tools/simplestreams.go:128: func MarshalToolsMetadataJSON(metadata
[]*ToolsMetadata) (index, products []byte, err error) {
Please move these functions to a separate file

[axw1, 2013-08-30 08:46:56]

https://codereview.appspot.com/13417043/diff/7001/cmd/plugins/juju-metadata/t...
File cmd/plugins/juju-metadata/toolsmetadata.go (right):

https://codereview.appspot.com/13417043/diff/7001/cmd/plugins/juju-metadata/t...
cmd/plugins/juju-metadata/toolsmetadata.go:30: const pathPrefix = "tools/"
On 2013/08/30 06:40:42, wallyworld wrote:
> I'd make this "releases"

Done.

https://codereview.appspot.com/13417043/diff/7001/cmd/plugins/juju-metadata/t...
cmd/plugins/juju-metadata/toolsmetadata.go:55: env, err :=
environs.NewFromName(c.EnvName)
On 2013/08/30 06:40:42, wallyworld wrote:
> There's little point using an env to read the tools - the tools are readily
> accessible directly from "https://juju-dist.s3.amazonaws.com/" (see
synctools).

Gotcha. Sorry, I missed the bit in sync-tools that did the magic. I'll move the
fileStorageReader out of there so I can reuse it.

https://codereview.appspot.com/13417043/diff/7001/cmd/plugins/juju-metadata/t...
cmd/plugins/juju-metadata/toolsmetadata.go:74: // it's from the public or
private storage at this point.
On 2013/08/30 06:40:42, wallyworld wrote:
> urlPath should have "releases" prepended.
> If the base url is http://juju.canonical.com/tools, then tools tarballs will
be
> at 
> releases/foo.tar.gz etc
> and simplestreams metadata will be at
> streams/v1/index.json etc
> 

Done.

https://codereview.appspot.com/13417043/diff/7001/environs/emptystorage.go
File environs/emptystorage.go (right):

https://codereview.appspot.com/13417043/diff/7001/environs/emptystorage.go#ne...
environs/emptystorage.go:17: var EmptyStorage Storage = struct {
On 2013/08/30 06:40:42, wallyworld wrote:
> Do we still need this given the implementation has changed?

No, I'll back it out.

https://codereview.appspot.com/13417043/diff/7001/environs/localstorage/stora...
File environs/localstorage/storage.go (right):

https://codereview.appspot.com/13417043/diff/7001/environs/localstorage/stora...
environs/localstorage/storage.go:71: }
On 2013/08/30 06:40:42, wallyworld wrote:
> What's this used for?

Drive-by fix. List shouldn't error if there are no matching files.

https://codereview.appspot.com/13417043/diff/7001/environs/simplestreams/simp...
File environs/simplestreams/simplestreams.go (right):

https://codereview.appspot.com/13417043/diff/7001/environs/simplestreams/simp...
environs/simplestreams/simplestreams.go:179: func (c *ItemCollection)
UnmarshalJSON(b []byte) error {
On 2013/08/30 06:40:42, wallyworld wrote:
> Please move this and associated code/tests to json.go

Done.

https://codereview.appspot.com/13417043/diff/7001/environs/simplestreams/simp...
File environs/simplestreams/simplestreams_test.go (right):

https://codereview.appspot.com/13417043/diff/7001/environs/simplestreams/simp...
environs/simplestreams/simplestreams_test.go:439: "c": 123
On 2013/08/30 06:40:42, wallyworld wrote:
> Could we please make this a big number to test the issue is solved

This test isn't verifying that problem. It's verifying that we get default JSON
unmarshalling behaviour for ItemCollection if you don't go through the
ParseCloudMetadata interface.

I've added an explicit test to environs/tools/simplestreams_test.go.

https://codereview.appspot.com/13417043/diff/7001/environs/tools/simplestream...
File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/13417043/diff/7001/environs/tools/simplestream...
environs/tools/simplestreams.go:24: ProductMetadataPath =
"streams/v1/com.ubuntu.juju:released:tools.json"
On 2013/08/30 06:40:42, wallyworld wrote:
> This is only used when generating data, please move it as per the comment
below
> and possibly unexport.

Moved, but not unexported. It's used in juju-metadata.

https://codereview.appspot.com/13417043/diff/7001/environs/tools/simplestream...
environs/tools/simplestreams.go:128: func MarshalToolsMetadataJSON(metadata
[]*ToolsMetadata) (index, products []byte, err error) {
On 2013/08/30 06:40:42, wallyworld wrote:
> Please move these functions to a separate file

Done.

[axw, 2013-08-30 08:49:39]

Please take a look.

[wallyworld, 2013-09-02 01:44:13]

LGTM with the issues highlighted addressed

https://codereview.appspot.com/13417043/diff/18001/environs/localstorage/stor...
File environs/localstorage/storage.go (right):

https://codereview.appspot.com/13417043/diff/18001/environs/localstorage/stor...
environs/localstorage/storage.go:71: }
I'd prefer returning an empty array here ie []string{}
Returning nil, nil seems wrong

https://codereview.appspot.com/13417043/diff/18001/environs/simplestreams/jso...
File environs/simplestreams/json.go (right):

https://codereview.appspot.com/13417043/diff/18001/environs/simplestreams/jso...
environs/simplestreams/json.go:20: type CloudMetadata struct {
Sorry if I was unclear, the structs which model the simplestreams data stay
where they were; it's only the json marshall/unmarshall code which goes in here

https://codereview.appspot.com/13417043/diff/18001/environs/simplestreams/jso...
File environs/simplestreams/json_test.go (right):

https://codereview.appspot.com/13417043/diff/18001/environs/simplestreams/jso...
environs/simplestreams/json_test.go:22: "c": 123
Please make this a big number so that we can ensure the fix actually works. I
know it's tested elsewhere too, but this code is where the implementation is

https://codereview.appspot.com/13417043/diff/18001/environs/simplestreams/jso...
environs/simplestreams/json_test.go:28: "c": float64(123),
why float when we want an int?

https://codereview.appspot.com/13417043/diff/18001/environs/tools/marshal.go
File environs/tools/marshal.go (right):

https://codereview.appspot.com/13417043/diff/18001/environs/tools/marshal.go#...
environs/tools/marshal.go:23: func MarshalToolsMetadataJSON(metadata
[]*ToolsMetadata) (index, products []byte, err error) {
Where are the tests for the methods in this file?

[axw1, 2013-09-02 02:46:03]

On 2013/09/02 01:44:13, wallyworld wrote:
> LGTM with the issues highlighted addressed
> 
>
https://codereview.appspot.com/13417043/diff/18001/environs/localstorage/stor...
> File environs/localstorage/storage.go (right):
> 
>
https://codereview.appspot.com/13417043/diff/18001/environs/localstorage/stor...
> environs/localstorage/storage.go:71: }
> I'd prefer returning an empty array here ie []string{}
> Returning nil, nil seems wrong

Done.

>
https://codereview.appspot.com/13417043/diff/18001/environs/simplestreams/jso...
> File environs/simplestreams/json.go (right):
> 
>
https://codereview.appspot.com/13417043/diff/18001/environs/simplestreams/jso...
> environs/simplestreams/json.go:20: type CloudMetadata struct {
> Sorry if I was unclear, the structs which model the simplestreams data stay
> where they were; it's only the json marshall/unmarshall code which goes in
here

Fixed. I've left "type itemCollection" (the ItemCollection clone), and
ItemCollection unmarshalling methods in json.go.

>
https://codereview.appspot.com/13417043/diff/18001/environs/simplestreams/jso...
> File environs/simplestreams/json_test.go (right):
> 
>
https://codereview.appspot.com/13417043/diff/18001/environs/simplestreams/jso...
> environs/simplestreams/json_test.go:22: "c": 123
> Please make this a big number so that we can ensure the fix actually works. I
> know it's tested elsewhere too, but this code is where the implementation is

As discussed on IRC, this is not testing the fix for numbers unmarshalling to
int64. This is testing what happens if someone attempts to un/marshal an
ItemCollection directly, not through ParseCloudMetadata.

>
https://codereview.appspot.com/13417043/diff/18001/environs/simplestreams/jso...
> environs/simplestreams/json_test.go:28: "c": float64(123),
> why float when we want an int?

As above. Default encoding/json behaviour for parsing a number into an
interface{} is to unmarshal as float64.

> https://codereview.appspot.com/13417043/diff/18001/environs/tools/marshal.go
> File environs/tools/marshal.go (right):
> 
>
https://codereview.appspot.com/13417043/diff/18001/environs/tools/marshal.go#...
> environs/tools/marshal.go:23: func MarshalToolsMetadataJSON(metadata
> []*ToolsMetadata) (index, products []byte, err error) {
> Where are the tests for the methods in this file?

On their way :)
There's tests in cmd/juju-metadata, just haven't migrated yet. Will do before
landing.

[axw, 2013-09-02 03:42:49]

Please take a look.

[jameinel, 2013-09-02 05:48:29]

https://codereview.appspot.com/13417043/diff/27001/cmd/plugins/juju-metadata/...
File cmd/plugins/juju-metadata/toolsmetadata_test.go (right):

https://codereview.appspot.com/13417043/diff/27001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/toolsmetadata_test.go:232: SHA256:  
"9268ba87201b1514171cc09334db6f680e1e013f5ae584f1b43252c743eea841",
How did you come up with these SHA sums? It certainly seems very site-specific
because the actual SHA will end up depending on the details of the compression
(by default gzip compression inserts date stamps into the stream, so you won't
ever be able to predict the compression of a newly generated file).

If we can validate them through some other path, I would love to see it, but if
we have to, you can do:

   SHA256: metadata[0].SHA256

Can we grab the actual .tgz before it is uploaded and check its sha that way?

[axw1, 2013-09-02 05:59:26]

https://codereview.appspot.com/13417043/diff/27001/cmd/plugins/juju-metadata/...
File cmd/plugins/juju-metadata/toolsmetadata_test.go (right):

https://codereview.appspot.com/13417043/diff/27001/cmd/plugins/juju-metadata/...
cmd/plugins/juju-metadata/toolsmetadata_test.go:232: SHA256:  
"9268ba87201b1514171cc09334db6f680e1e013f5ae584f1b43252c743eea841",
On 2013/09/02 05:48:30, jameinel wrote:
> How did you come up with these SHA sums? It certainly seems very site-specific
> because the actual SHA will end up depending on the details of the compression
> (by default gzip compression inserts date stamps into the stream, so you won't
> ever be able to predict the compression of a newly generated file).

I've changed this a bit already, so the contents of the files are not tgz, but a
well defined content string. So the SHA1 sum is just a sum of that content.

> If we can validate them through some other path, I would love to see it, but
if
> we have to, you can do:
> 
>    SHA256: metadata[0].SHA256

What would be the point? I'm checking the value of metadata[N] :)

> Can we grab the actual .tgz before it is uploaded and check its sha that way?

Yes, that's probably more sensible. I'll do that.