simplestreams tools lookup on major.minor version

simplestreams tools lookup on major.minor version

Juju's simplestreams support required exact matches on product id.
The tools lookup functionality however requires inexact matches on
major.minor version number. This branch enhances the simplestreams
support to allow such inexact matches. The tools validation plugin
is enhanced to use the new feature. This work is a pre-req for
replacing the legacy tools lookup with simplestreams.

https://code.launchpad.net/~wallyworld/juju-core/inexact-simplestreams-tools-match/+merge/182792

Requires: https://code.launchpad.net/~wallyworld/juju-core/bootstrap-exact-tools/+merge/182378

(do not edit description out of merge proposal)

[wallyworld, 2013-08-29 00:58:55]

Please take a look.

[jameinel, 2013-08-29 09:00:00]

I'm really hesitant to have an API take a string that "might be an exact value"
or "might be a regex".

https://codereview.appspot.com/13369043/diff/1/environs/simplestreams/simples...
File environs/simplestreams/simplestreams.go (right):

https://codereview.appspot.com/13369043/diff/1/environs/simplestreams/simples...
environs/simplestreams/simplestreams.go:647: 
do we really want something that "might be an exact string" or "might be a
regex"?

Especially given the "com.ubuntu...." syntax where all of those "." symbols will
be interpreted as wildcards instead of literals.

I could see some sort of "prefix match", or "always a regex" and maybe a higher
level function that takes an exact match and turns it into an exact regex match.

I realize that it is a little unlikely to have an accidental match, at least as
long as we never use '*' or '+' in the strings. But it just feels like something
that might bite us and we would be stuck with a WTF moment.

So I think I would push for "productIdMatches always treats matchExpr as a
regex" and we can move the compiling of the value out of this function (to avoid
compiling N times for N product ids).
And just have the higher up functions that take an exact match produce an exact
regex.

[fwereade, 2013-08-30 06:46:45]

I have a few questions here. I may be missing something.

https://codereview.appspot.com/13369043/diff/1/cmd/plugins/juju-metadata/vali...
File cmd/plugins/juju-metadata/validatetoolsmetadata.go (right):

https://codereview.appspot.com/13369043/diff/1/cmd/plugins/juju-metadata/vali...
cmd/plugins/juju-metadata/validatetoolsmetadata.go:102: 
I'm really not loving this user interface. What sort of scope do we have for a
rework here? I'm thinking something like:

$ juju metadata validate-tools
# matches version.Current's M.M.x.x

$ juju metadata validate-tools -j 1
# makes no sense, pure major matches don't help anyone

$ juju metadata validate-tools -j 1.4
# matches 1.4.x.x

$ juju metadata validate-tools -j 1.4.0.4
# matches 1.4.0.4

...but maybe there are use cases I'm missing?

https://codereview.appspot.com/13369043/diff/1/cmd/plugins/juju-metadata/vali...
cmd/plugins/juju-metadata/validatetoolsmetadata.go:125: 
do we just drop components after minor? They should either be handled or cause
errors.

https://codereview.appspot.com/13369043/diff/1/environs/simplestreams/simples...
File environs/simplestreams/simplestreams.go (right):

https://codereview.appspot.com/13369043/diff/1/environs/simplestreams/simples...
environs/simplestreams/simplestreams.go:647: 
On 2013/08/29 09:00:00, jameinel wrote:
> do we really want something that "might be an exact string" or "might be a
> regex"?
> 
> Especially given the "com.ubuntu...." syntax where all of those "." symbols
will
> be interpreted as wildcards instead of literals.
> 
> I could see some sort of "prefix match", or "always a regex" and maybe a
higher
> level function that takes an exact match and turns it into an exact regex
match.
> 
> I realize that it is a little unlikely to have an accidental match, at least
as
> long as we never use '*' or '+' in the strings. But it just feels like
something
> that might bite us and we would be stuck with a WTF moment.
> 
> So I think I would push for "productIdMatches always treats matchExpr as a
> regex" and we can move the compiling of the value out of this function (to
avoid
> compiling N times for N product ids).
> And just have the higher up functions that take an exact match produce an
exact
> regex.

Agree that this is a timebomb. Can we maybe even just pass in a func(string)
bool to do the actual matching?

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

https://codereview.appspot.com/13369043/diff/1/environs/tools/simplestreams.g...
environs/tools/simplestreams.go:43: 
In line with the general idea that we should pick one name per concept in the
project, I'd really like us to stop calling these things constraints.

https://codereview.appspot.com/13369043/diff/1/environs/tools/simplestreams.g...
environs/tools/simplestreams.go:51: 
I'm confused... the same tools for different arches are different products, but
the versions built for different platforms are the same product? I could
understand one product per arch+platform per version, or one product per version
with a whole load of platform+arch variants, but I don't see the advantages of
mixing the approaches.

https://codereview.appspot.com/13369043/diff/1/environs/tools/simplestreams.g...
environs/tools/simplestreams.go:56: 
This is starting to feel a bit inside-out. Surely we do still need the
tools.List functionality..? This seems like it's designed to replace that, but
in the short/medium term we're going to want to cache information about
available tools in the API server (at least; maybe in state, in fact).

A single match func seems most suitable for producing the sorts of lists we're
most likely to need when querying directly against simplestreams.

[wallyworld, 2013-08-30 09:21:25]

Please take a look.

https://codereview.appspot.com/13369043/diff/1/cmd/plugins/juju-metadata/vali...
File cmd/plugins/juju-metadata/validatetoolsmetadata.go (right):

https://codereview.appspot.com/13369043/diff/1/cmd/plugins/juju-metadata/vali...
cmd/plugins/juju-metadata/validatetoolsmetadata.go:102: 
This utility is for users to see what's there.

On 2013/08/30 06:46:45, fwereade wrote:
> I'm really not loving this user interface. What sort of scope do we have for a
> rework here? I'm thinking something like:
> 
> $ juju metadata validate-tools
> # matches version.Current's M.M.x.x
> 

No, without params it will list all tools.

> $ juju metadata validate-tools -j 1
> # makes no sense, pure major matches don't help anyone
>

This is for users who want to see what version 1.x or 2.x tools are there.
 
> $ juju metadata validate-tools -j 1.4
> # matches 1.4.x.x
> 

That's what -m does currently. I could rework it so that -m is removed and -j
used for everything

> $ juju metadata validate-tools -j 1.4.0.4
> # matches 1.4.0.4
> 

Yes. That's the current behaviour

https://codereview.appspot.com/13369043/diff/1/cmd/plugins/juju-metadata/vali...
cmd/plugins/juju-metadata/validatetoolsmetadata.go:125: 
On 2013/08/30 06:46:45, fwereade wrote:
> do we just drop components after minor? They should either be handled or cause
> errors.

I'll raise an error

https://codereview.appspot.com/13369043/diff/1/environs/simplestreams/simples...
File environs/simplestreams/simplestreams.go (right):

https://codereview.appspot.com/13369043/diff/1/environs/simplestreams/simples...
environs/simplestreams/simplestreams.go:647: 
On 2013/08/30 06:46:45, fwereade wrote:
> On 2013/08/29 09:00:00, jameinel wrote:
> > do we really want something that "might be an exact string" or "might be a
> > regex"?
> > 
> > Especially given the "com.ubuntu...." syntax where all of those "." symbols
> will
> > be interpreted as wildcards instead of literals.
> > 
> > I could see some sort of "prefix match", or "always a regex" and maybe a
> higher
> > level function that takes an exact match and turns it into an exact regex
> match.
> > 
> > I realize that it is a little unlikely to have an accidental match, at least
> as
> > long as we never use '*' or '+' in the strings. But it just feels like
> something
> > that might bite us and we would be stuck with a WTF moment.
> > 
> > So I think I would push for "productIdMatches always treats matchExpr as a
> > regex" and we can move the compiling of the value out of this function (to
> avoid
> > compiling N times for N product ids).
> > And just have the higher up functions that take an exact match produce an
> exact
> > regex.
> 
> Agree that this is a timebomb. Can we maybe even just pass in a func(string)
> bool to do the actual matching?

It's not a time bomb given the strict format of the ids, none the less, I've
reworked it so that exact matches are converted to regexp and only regexp
matching is used. Given the small number of matches being done, it's not worth
the extra code and potentially considerable complexity to try and cache the
compiled expressions.

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

https://codereview.appspot.com/13369043/diff/1/environs/tools/simplestreams.g...
environs/tools/simplestreams.go:43: 
On 2013/08/30 06:46:45, fwereade wrote:
> In line with the general idea that we should pick one name per concept in the
> project, I'd really like us to stop calling these things constraints.

Well, it is a constraint though isn't it? As in it constrains what tools are
loaded.

https://codereview.appspot.com/13369043/diff/1/environs/tools/simplestreams.g...
environs/tools/simplestreams.go:51: 
On 2013/08/30 06:46:45, fwereade wrote:
> I'm confused... the same tools for different arches are different products,
but
> the versions built for different platforms are the same product? I could
> understand one product per arch+platform per version, or one product per
version
> with a whole load of platform+arch variants, but I don't see the advantages of
> mixing the approaches.

Easier to discuss this stuff in person.

https://codereview.appspot.com/13369043/diff/1/environs/tools/simplestreams.g...
environs/tools/simplestreams.go:56: 
On 2013/08/30 06:46:45, fwereade wrote:
> This is starting to feel a bit inside-out. Surely we do still need the
> tools.List functionality..? This seems like it's designed to replace that, but
> in the short/medium term we're going to want to cache information about
> available tools in the API server (at least; maybe in state, in fact).
> 
> A single match func seems most suitable for producing the sorts of lists we're
> most likely to need when querying directly against simplestreams.

Ditto

[jameinel, 2013-09-01 11:42:12]

LGTM

I have some comments, but we could just land this (progress not perfection). But
see if any of them resonate with you.

https://codereview.appspot.com/13369043/diff/15001/environs/imagemetadata/sim...
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/13369043/diff/15001/environs/imagemetadata/sim...
environs/imagemetadata/simplestreams.go:48: ids[i] =
regexp.QuoteMeta(fmt.Sprintf("com.ubuntu.cloud%s:server:%s:%s", stream, version,
arch))
Given this bug:
https://bugs.launchpad.net/juju-core/+bug/1219123

Should we be checking the value of 'stream' and be setting it to the empty
string if it is 'released' ?

https://codereview.appspot.com/13369043/diff/15001/environs/simplestreams/sim...
File environs/simplestreams/simplestreams.go (left):

https://codereview.appspot.com/13369043/diff/15001/environs/simplestreams/sim...
environs/simplestreams/simplestreams.go:40: Ids() ([]string, error)
Since this is now a slice of regexes about Ids, can we change the name so it
doesn't cause confusion? (IdRegexes ?)

https://codereview.appspot.com/13369043/diff/15001/environs/tools/simplestrea...
File environs/tools/simplestreams.go (left):

https://codereview.appspot.com/13369043/diff/15001/environs/tools/simplestrea...
environs/tools/simplestreams.go:37: func (tc *ToolsConstraint) Ids() ([]string,
error) {
I'm guessing this function should be renamed as well.
Though it makes me wonder why we couldn't just compile the regexes at this point
and be done with it, rather than passing it around as strings that we then
compile for each id that we compare against.

If you're happier with the current layering that is fine.

https://codereview.appspot.com/13369043/diff/15001/environs/tools/simplestrea...
File environs/tools/simplestreams.go (right):

https://codereview.appspot.com/13369043/diff/15001/environs/tools/simplestrea...
environs/tools/simplestreams.go:112: tmNumber := version.MustParse(tm.Version)
I'm a little concerned about MustParse, since that will generate panics() if
something fails. Can we handle an error and just continue or report an error
instead of panicing ?

[wallyworld, 2013-09-02 04:02:50]

I've pushed changes which rework the format of the simplestreams metadata for
tools. The grouping is now based on series:arch like it is for image metadata.
This eliminates the need for any regexp code. I'll get the changes through
Scott's group.