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 {
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/imagemetadata.go#newcode131
cmd/plugins/juju-metadata/imagemetadata.go:131: %q
why are the directories quoted?

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()
Why clear the entire environment?

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")
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/imagemetadata_test.go#newcode49
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/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)
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/imagemetadata_test.go#newcode129
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/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() })
s.PatchEnvironment("AWS_ACCESS_KEY_ID", "access")

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", "")
s.PatchEnvironment

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
Do you think it is worthwhile adding any logging to the failure
conditions?

https://codereview.appspot.com/14663043/diff/4001/environs/imagemetadata/generate.go#newcode57
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/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{}
all test suits should contain from testbase.LoggingSuite to capture
logging (and give cleanup methods)

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)
c.Assert(metadata, gc.HasLen, 1)

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)
gc.HasLen

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)
gc.HasLen

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)
and again

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)
last one?

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{}
testbase.LoggingSuite

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