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

Revision history for this message
Ian Booth (wallyworld) wrote :

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

https://codereview.appspot.com/14540055/diff/4001/environs/imagemetadata/generate.go#newcode15
environs/imagemetadata/generate.go:15: metadataInfo, err :=
generateMetadata(series, im, cloudSpec)
On 2013/10/16 01:26:11, thumper wrote:
> This has me queezy.

> Surely WriteMetadata would take the metadataInfo as an input
parameter?

The sematics of the write function is to take a ImageMetadata struct and
write out the simplestreams metadata. In order to write the metadata, it
needs to be generated from the struct. The method could be called
WriteAndGenerate I guess. But the generation and writing to file is an
atomic operation.

A subsequent branch renames the method anyway and refactors the content
to align with what Andrew did for tools metadata.

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

https://codereview.appspot.com/14540055/diff/4001/environs/imagemetadata/generate_test.go#newcode17
environs/imagemetadata/generate_test.go:17: type generateSuite struct{}
On 2013/10/16 01:26:11, thumper wrote:
> testbase.LoggingSuite

Will fix

https://codereview.appspot.com/14540055/diff/4001/environs/imagemetadata/generate_test.go#newcode34
environs/imagemetadata/generate_test.go:34: c.Assert(len(metadata),
gc.Equals, 1)
On 2013/10/16 01:26:11, thumper wrote:
> gc.HasLen

Will fix

https://codereview.appspot.com/14540055/diff/4001/environs/imagemetadata/simplestreams.go
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/14540055/diff/4001/environs/imagemetadata/simplestreams.go#newcode121
environs/imagemetadata/simplestreams.go:121: Release string
`json:"-"`
On 2013/10/16 01:26:11, thumper wrote:
> What does json:"-" do?

Tells json not to marshall that attribute. It's there to provide info
for other mthods in the struct to do their thing, but we don't want it
serialised to json.

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

https://codereview.appspot.com/14540055/diff/4001/environs/tools/marshal.go#newcode69
environs/tools/marshal.go:69: itemsversion := updated.Format("20060102")
// YYYYMMDD
On 2013/10/16 01:26:11, thumper wrote:
> Some of these changes are the same as the branch I just reviewed.

I fixed this in a downstream branch - I didn't pump the changes back up
the pipe sorry.

https://codereview.appspot.com/14540055/

« Back to merge proposal