Code review comment for lp:~axwalk/juju-core/lp1235717-refactor-simplestreams-writemetadata

This looks great in general, with a few remarks and suggestions below.

https://codereview.appspot.com/14527043/diff/5001/cmd/plugins/juju-metadata/toolsmetadata.go
File cmd/plugins/juju-metadata/toolsmetadata.go (right):

https://codereview.appspot.com/14527043/diff/5001/cmd/plugins/juju-metadata/toolsmetadata.go#newcode76
cmd/plugins/juju-metadata/toolsmetadata.go:76: return
tools.MergeAndWriteMetadata(targetStorage, toolsList,
tools.ResolveFlag(c.fetch))
I'd prefer to see explicit values for the resolve flag here - using it
as a boolean gets around the advantages of using the named constants, I
think.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go
File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode217
environs/tools/simplestreams.go:217: // the metadata locally.
s/metadata/size and hash/ ?

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode220
environs/tools/simplestreams.go:220: if md.Size == 0 {
if md.Size != 0 {
     continue
}

to save an indent level?

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode240
environs/tools/simplestreams.go:240: func MergeMetadata(newMetadata,
oldMetadata []*ToolsMetadata) []*ToolsMetadata {
I'm not sure that "new" and "old" are appropriate prefixes here - they
seem to imply that the arguments are treated differently, but isn't
MergeMetadata commutative?

Personally I'd use something like m0 and m1, but go with your own
thoughts for naming.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode292
environs/tools/simplestreams.go:292: type ResolveFlag bool
I know we do this elsewhere, but I've come to dislike
this idiom - it's an unsatisfactory halfway house between
named constants and bools. There's still code that
uses the bool semantics, which means that these names aren't
really helping much because you have to know their values
anyway.

I'd prefer either to use int and iota or to stick with the bool
and lose the named constants.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode300
environs/tools/simplestreams.go:300: // and merges it with metadata
generated from the given tools list.
It's not clear what targetTools is here. It sounds like it's
a list of tools in the target storage, but it doesn't actually
seem like that's the case.

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode301
environs/tools/simplestreams.go:301: // If resolve is true, incomplete
metadata is resolved by fetching the tools
// If resolve is Resolve

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode304
environs/tools/simplestreams.go:304: func MergeAndWriteMetadata(stor
storage.Storage, targetTools coretools.List, resolve ResolveFlag) error
{
s/stor/target/ ?

(given that we refer to "the target storage" above)

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams_test.go
File environs/tools/simplestreams_test.go (right):

https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams_test.go#newcode561
environs/tools/simplestreams_test.go:561: func (*metadataHelperSuite)
TestMergeMetadata(c *gc.C) {
I think this might read better as a table-driven test,
with each item in the table containing the inputs and the
expected output.
Then it's easy to check commutativity in a uniform
way, for example.

https://codereview.appspot.com/14527043/