Merge new image metadata with existing

Merge new image metadata with existing

When image id metadata is generated, any existing metadata is
now loaded and then the new metadata is merged in. This allows
the generate-image plugin to be run multiple times to build up
metadata for multiple images. Some fairly extensive refactoring
to how image metadata search constraints are set up was required.

https://code.launchpad.net/~wallyworld/juju-core/merge-new-image-metadata/+merge/190898

(do not edit description out of merge proposal)

[wallyworld, 2013-10-14 09:42:18]

Please take a look.

[wallyworld, 2013-10-15 00:46:52]

Please take a look.

[thumper, 2013-10-16 00:47:59]

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
File cmd/plugins/juju-metadata/imagemetadata.go (right):

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata.go:64: if store, err :=
configstore.Default(); err == nil {
What should we be doing if there is no configstore? Is it an error?

It seems that either we allow to use to say "don't use an environment" or we
should fail if we can't open the environment defined by the command.

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata.go:131: %q
why are the directories quoted?

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
File cmd/plugins/juju-metadata/imagemetadata_test.go (right):

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata_test.go:39: os.Clearenv()
Why clear the entire environment?

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata_test.go:42: restore :=
testbase.PatchEnvironment("AWS_ACCESS_KEY_ID", "access")
just use:

s.PatchEnvironment("AWS_ACCESS_KEY_ID", "access")

This adds the restore cleanup for you.

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata_test.go:49: for _, envstring := range
s.environ {
I'd be tempted to make this a function by itself, and call "addCleanup" just
after the os.Clearenv().

Adding the home.Restore() as a cleanup also means you don't need a TestTearDown.

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata_test.go:88:
c.Assert(strings.Contains(content, fmt.Sprintf(`"region": %q`,
expected.region)), jc.IsTrue)
Do you realise that we have a checker for Contains?

c.Assert(content, jc.Contains, fmt.Sprintf(`"region": %q`, expected.region))

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata_test.go:129: errOut :=
ctx.Stdout.(*bytes.Buffer).String()
there is also testing.Stdout(ctx) which does the horrible cast for you.

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/v...
File cmd/plugins/juju-metadata/validateimagemetadata_test.go (right):

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/v...
cmd/plugins/juju-metadata/validateimagemetadata_test.go:101:
s.AddCleanup(func(*gc.C) { restore() })
s.PatchEnvironment("AWS_ACCESS_KEY_ID", "access")

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/v...
cmd/plugins/juju-metadata/validateimagemetadata_test.go:133:
testbase.PatchEnvironment("AWS_ACCESS_KEY_ID", "")
s.PatchEnvironment

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
File environs/imagemetadata/generate.go (right):

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate.go:27: return err
Do you think it is worthwhile adding any logging to the failure conditions?

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate.go:57: newRecord := im
Just to be clear, this isn't making a new record, but modifying the pointer of
the value passed in.  Is that what you want?

To make sure it is a copy you could do:

  newRecord := *im
  newRecord.blah = blah
  toWrite[i] = &newRecord

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
File environs/imagemetadata/generate_test.go (right):

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:20: type generateSuite struct{}
all test suits should contain from testbase.LoggingSuite to capture logging (and
give cleanup methods)

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:32: c.Assert(len(metadata), gc.Equals,
1)
c.Assert(metadata, gc.HasLen, 1)

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:54: c.Assert(len(metadata), gc.Equals,
1)
gc.HasLen

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:93: c.Assert(len(metadata), gc.Equals,
2)
gc.HasLen

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:135: c.Assert(len(metadata), gc.Equals,
3)
and again

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:184: c.Assert(len(metadata), gc.Equals,
3)
last one?

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/mars...
File environs/imagemetadata/marshal_test.go (right):

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/mars...
environs/imagemetadata/marshal_test.go:17: type marshalSuite struct{}
testbase.LoggingSuite

[wallyworld, 2013-10-16 01:52:13]

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
File cmd/plugins/juju-metadata/imagemetadata.go (right):

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata.go:64: if store, err :=
configstore.Default(); err == nil {
On 2013/10/16 00:47:59, thumper wrote:
> What should we be doing if there is no configstore? Is it an error?
> 

No. It just means that the user wants to run the command without an environment.
It is not an error as far as this command is concerned.

> It seems that either we allow to use to say "don't use an environment" or we
> should fail if we can't open the environment defined by the command.

I'd rather keep the semantics simple for the user. A written, the command works
quite nicely without an environment, so long as all the parameters are
specified. But it does complain if an env name is specified and the env cannot
be opened.

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata.go:131: %q
On 2013/10/16 00:47:59, thumper wrote:
> why are the directories quoted?

Seemed like a good idea. I can unquote.

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
File cmd/plugins/juju-metadata/imagemetadata_test.go (right):

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata_test.go:39: os.Clearenv()
On 2013/10/16 00:47:59, thumper wrote:
> Why clear the entire environment?

We don't need *any* env vars set for the test and there's so many combinations
of AWS and EC2 env vars it's just easier.

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata_test.go:42: restore :=
testbase.PatchEnvironment("AWS_ACCESS_KEY_ID", "access")
On 2013/10/16 00:47:59, thumper wrote:
> just use:
> 
> s.PatchEnvironment("AWS_ACCESS_KEY_ID", "access")
> 
> This adds the restore cleanup for you.

will do

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata_test.go:88:
c.Assert(strings.Contains(content, fmt.Sprintf(`"region": %q`,
expected.region)), jc.IsTrue)
On 2013/10/16 00:47:59, thumper wrote:
> Do you realise that we have a checker for Contains?
> 
> c.Assert(content, jc.Contains, fmt.Sprintf(`"region": %q`, expected.region))

I do now

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/i...
cmd/plugins/juju-metadata/imagemetadata_test.go:129: errOut :=
ctx.Stdout.(*bytes.Buffer).String()
On 2013/10/16 00:47:59, thumper wrote:
> there is also testing.Stdout(ctx) which does the horrible cast for you.

\o/

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/v...
File cmd/plugins/juju-metadata/validateimagemetadata_test.go (right):

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/v...
cmd/plugins/juju-metadata/validateimagemetadata_test.go:101:
s.AddCleanup(func(*gc.C) { restore() })
On 2013/10/16 00:47:59, thumper wrote:
> s.PatchEnvironment("AWS_ACCESS_KEY_ID", "access")

will fix

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/v...
cmd/plugins/juju-metadata/validateimagemetadata_test.go:133:
testbase.PatchEnvironment("AWS_ACCESS_KEY_ID", "")
On 2013/10/16 00:47:59, thumper wrote:
> s.PatchEnvironment

will fix

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
File environs/imagemetadata/generate.go (right):

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate.go:27: return err
On 2013/10/16 00:47:59, thumper wrote:
> Do you think it is worthwhile adding any logging to the failure conditions?

Could do although the error will be propagated and printed higer up.

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate.go:57: newRecord := im
On 2013/10/16 00:47:59, thumper wrote:
> Just to be clear, this isn't making a new record, but modifying the pointer of
> the value passed in.  Is that what you want?
> 
> To make sure it is a copy you could do:
> 
>   newRecord := *im
>   newRecord.blah = blah
>   toWrite[i] = &newRecord

Bah, the slice did contain records and it was changed and I missed this. Good
pickup.

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
File environs/imagemetadata/generate_test.go (right):

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:20: type generateSuite struct{}
On 2013/10/16 00:47:59, thumper wrote:
> all test suits should contain from testbase.LoggingSuite to capture logging
(and
> give cleanup methods)

will fix

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:32: c.Assert(len(metadata), gc.Equals,
1)
On 2013/10/16 00:47:59, thumper wrote:
> c.Assert(metadata, gc.HasLen, 1)

will fix

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:54: c.Assert(len(metadata), gc.Equals,
1)
On 2013/10/16 00:47:59, thumper wrote:
> gc.HasLen

will fix

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:93: c.Assert(len(metadata), gc.Equals,
2)
On 2013/10/16 00:47:59, thumper wrote:
> gc.HasLen

will fix

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:135: c.Assert(len(metadata), gc.Equals,
3)
On 2013/10/16 00:47:59, thumper wrote:
> and again

ditto

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/gene...
environs/imagemetadata/generate_test.go:184: c.Assert(len(metadata), gc.Equals,
3)
On 2013/10/16 00:47:59, thumper wrote:
> last one?

ditto

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/mars...
File environs/imagemetadata/marshal_test.go (right):

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/mars...
environs/imagemetadata/marshal_test.go:17: type marshalSuite struct{}
On 2013/10/16 00:47:59, thumper wrote:
> testbase.LoggingSuite

Will fix

[wallyworld, 2013-10-16 02:51:32]

Please take a look.

[wallyworld, 2013-10-16 04:41:10]

Please take a look.

[thumper, 2013-10-17 04:06:50]

LGTM