Code review comment for lp:~wallyworld/juju-core/merge-new-image-metadata

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

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/imagemetadata.go#newcode64
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/imagemetadata.go#newcode131
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/imagemetadata_test.go
File cmd/plugins/juju-metadata/imagemetadata_test.go (right):

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/imagemetadata_test.go#newcode39
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/imagemetadata_test.go#newcode42
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/imagemetadata_test.go#newcode88
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/imagemetadata_test.go#newcode129
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/validateimagemetadata_test.go
File cmd/plugins/juju-metadata/validateimagemetadata_test.go (right):

https://codereview.appspot.com/14663043/diff/4001/cmd/plugins/juju-metadata/validateimagemetadata_test.go#newcode101
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/validateimagemetadata_test.go#newcode133
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/generate.go
File environs/imagemetadata/generate.go (right):

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/generate.go#newcode27
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/generate.go#newcode57
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/generate_test.go
File environs/imagemetadata/generate_test.go (right):

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/generate_test.go#newcode20
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/generate_test.go#newcode32
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/generate_test.go#newcode54
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/generate_test.go#newcode93
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/generate_test.go#newcode135
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/generate_test.go#newcode184
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/marshal_test.go
File environs/imagemetadata/marshal_test.go (right):

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

Will fix

https://codereview.appspot.com/14663043/