On 2013/10/09 16:29:42, rog wrote:
> LGTM with some thoughts below.

Thanks. I'll change the newMetadata/oldMetadata thing as suggested, but
I think I'll leave the others to a followup.

> For the future, I think that the "resolve" mode is unnecessary and
should go -
> if we have items in the target bucket that we don't have hashes for,
we should
> just ignore them. This will actually work fine in practice, I think,
even if we
> abort a SyncTools half way through, because when we retry the
SyncTools, any
> items in the target bucket that we didn't get around to adding to the
target
> bucket simplestreams data should have metadata in the source bucket's
> simplestreams data.

> In general, I think retro-discovering hash data by trying to find out
what the
> data currently is is a flawed idea - the hash of some tools should be
generated
> on first upload and preserved forever after.

I'd like to say I agree, however I know the requirements here are a bit
subtle.
I think it'd be good to talk with wallyworld at SFO to nut it out.


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#newcode240
> environs/tools/simplestreams.go:240: func MergeMetadata(newMetadata,
oldMetadata
> []*ToolsMetadata) []*ToolsMetadata {
> On 2013/10/09 14:28:25, axw wrote:
> > On 2013/10/09 12:08:57, rog wrote:
> > > 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.
> >
> > It's not commutative, per the last sentence of the doc comment:
> > "If both entries have information, prefer the entry from
newMetadata".
> >
> > The idea behind that is that if both have size&hash, then the one
from
> > "newMetadata" is considered newer. That's to cater for copying over
the top of
> > existing tools (say, a dev build).

> Hmm, that *should* never happen - a version should always correspond
only to one
> particular tarball. I think if we do get a clash like this, then we
have a
> problem that I suspect our current logic won't cope with.

> Perhaps this function should check for inconsistencies and return an
error if it
> finds one.

Yeah, that sounds sensible. The tools won't be copied over if they are
already in the target, so the "newMetadata" entry won't have a
size/hash. So this code is only handling a theoretical case, anyway.
I'll make this change and repropose.


https://codereview.appspot.com/14527043/diff/5001/environs/tools/simplestreams.go#newcode292
> environs/tools/simplestreams.go:292: type ResolveFlag bool
> On 2013/10/09 14:28:25, axw wrote:
> > On 2013/10/09 12:08:57, rog wrote:
> > > 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.
> >
> > Consider a bool as (conceptually) a 1-bit unsigned int; then what's
the
> > difference between that and int with constant named values? The only
> difference
> > I see is that the bool is range limited to [0, 1], which is a very
good thing.

> The main difference I see is that various pieces of code will use a
boolean
> directly, or a boolean test and the sense of the code isn't clear in
that case.
> For example, in another piece of code in this CL, there's a boolean
field called
> "fetch" which is passed directly as the resolve argument; and in
> MergeAndWriteMetadata itself, we just use it as a bool.

> I'd leave ResolveFlag, but explicitly use the named constants
everywhere,
> rather than just type converting to ResolveFlag.

> But given that this function isn't long for this world (I'm hoping),
I'm ok as
> it is.


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


https://codereview.appspot.com/14527043/diff/13001/environs/tools/simplestreams_test.go#newcode641
> environs/tools/simplestreams_test.go:641: c.Assert(merged,
gc.DeepEquals,
> test.merged)
> Thanks - I find this much easier to follow (and it's easier to change
if we
> decide to change the semantics of MergeMetadata)


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