bootstrap: add --fake-series

bootstrap: add --fake-series

This allows developers to distribute multiple copies of their locally-built
tools to their private bucket, varying only by claimed series. So long as
the tools are in fact compatible across series, this allows us to run/test
mixed-series environments by specifying up-front what series we plan to use.

This does not address the need for similar changes to upgrade-juju: the
logic in there scares me a little, and it doesn't *need* to be fixed in
this CL.

To make --fake-series parsing consistent, I exported ValidSeries from the
charm package; and then to make that package consistent, I exported both
ValidUser and ValidName as well, and wrote some simple tests for them.

https://code.launchpad.net/~fwereade/juju-core/bootstrap-fake-series/+merge/156997

(do not edit description out of merge proposal)

[fwereade, 2013-04-03 23:40:17]

Please take a look.

[thumper, 2013-04-04 01:53:08]

LGTM - just few comments and questions below.

https://codereview.appspot.com/8352043/diff/1/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/8352043/diff/1/charm/url.go#newcode34
charm/url.go:34: var ValidUser = regexp.MustCompile("^[a-z0-9][a-zA-Z0-9+.-]+$")
Would be nice to have comments here now these are exported.

https://codereview.appspot.com/8352043/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/8352043/diff/1/cmd/juju/bootstrap.go#newcode19
cmd/juju/bootstrap.go:19: UploadTools bool
Is there a reason that Constraints, UploadTools and FakeSeries are exported?

https://codereview.appspot.com/8352043/diff/1/cmd/juju/bootstrap.go#newcode34
cmd/juju/bootstrap.go:34: f.Var(seriesVar{&c.FakeSeries}, "fake-series", "clone
uploaded tools for supplied serieses")
I have to admit that the seriesVar bit made this all look a bit too magical.

Why not just have a method turned a string into an array of strings?

[dave_cheney.net, 2013-04-04 07:36:37]

https://codereview.appspot.com/8352043/diff/1/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/8352043/diff/1/charm/url.go#newcode34
charm/url.go:34: var ValidUser = regexp.MustCompile("^[a-z0-9][a-zA-Z0-9+.-]+$")
On 2013/04/04 01:53:08, thumper wrote:
> Would be nice to have comments here now these are exported.

+1

https://codereview.appspot.com/8352043/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/8352043/diff/1/cmd/juju/bootstrap.go#newcode19
cmd/juju/bootstrap.go:19: UploadTools bool
On 2013/04/04 01:53:08, thumper wrote:
> Is there a reason that Constraints, UploadTools and FakeSeries are exported?

This is a main package, a command, so nothing can import them. Id say imported
or exported are dealers choice.

https://codereview.appspot.com/8352043/diff/1/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/8352043/diff/1/cmd/juju/upgradejuju_test.go#ne...
cmd/juju/upgradejuju_test.go:173: c.Logf("\ntest %d: %s\n", i, test.about)
I don't think either of the \n's are needed.

[dimitern, 2013-04-04 08:56:38]

LGTM modulo the trivials below.

https://codereview.appspot.com/8352043/diff/1/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/8352043/diff/1/charm/url.go#newcode34
charm/url.go:34: var ValidUser = regexp.MustCompile("^[a-z0-9][a-zA-Z0-9+.-]+$")
In addition to comments, maybe put all 3 in a single var block?

https://codereview.appspot.com/8352043/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/8352043/diff/1/cmd/juju/bootstrap.go#newcode26
cmd/juju/bootstrap.go:26: Purpose: "start up an environment from scratch",
It's about time to add some help doc text and describe the flags (see e.g.
upgrade-charm).

https://codereview.appspot.com/8352043/diff/1/cmd/juju/bootstrap.go#newcode34
cmd/juju/bootstrap.go:34: f.Var(seriesVar{&c.FakeSeries}, "fake-series", "clone
uploaded tools for supplied serieses")
s/serieses/comma-delimited list of series/ ?

https://codereview.appspot.com/8352043/diff/1/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/8352043/diff/1/cmd/juju/upgradejuju_test.go#ne...
cmd/juju/upgradejuju_test.go:173: c.Logf("\ntest %d: %s\n", i, test.about)
On 2013/04/04 07:36:37, dfc wrote:
> I don't think either of the \n's are needed.

+1, except probably one of them, in case you want to see the NL in the log dump.

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

https://codereview.appspot.com/8352043/diff/1/environs/tools.go#newcode113
environs/tools.go:113: log.Infof("environs: built tools %v (%dkB)",
toolsVersion, (size+512)/1024)
what's the magic 512 here?

https://codereview.appspot.com/8352043/diff/1/environs/tools_test.go
File environs/tools_test.go (right):

https://codereview.appspot.com/8352043/diff/1/environs/tools_test.go#newcode142
environs/tools_test.go:142: 
d

[fwereade, 2013-04-04 13:46:59]

*** Submitted:

bootstrap: add --fake-series

This allows developers to distribute multiple copies of their locally-built
tools to their private bucket, varying only by claimed series. So long as
the tools are in fact compatible across series, this allows us to run/test
mixed-series environments by specifying up-front what series we plan to use.

This does not address the need for similar changes to upgrade-juju: the
logic in there scares me a little, and it doesn't *need* to be fixed in
this CL.

To make --fake-series parsing consistent, I exported ValidSeries from the
charm package; and then to make that package consistent, I exported both
ValidUser and ValidName as well, and wrote some simple tests for them.

R=thumper, dfc, dimitern
CC=
https://codereview.appspot.com/8352043

https://codereview.appspot.com/8352043/diff/1/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/8352043/diff/1/charm/url.go#newcode34
charm/url.go:34: var ValidUser = regexp.MustCompile("^[a-z0-9][a-zA-Z0-9+.-]+$")
On 2013/04/04 08:56:38, dimitern wrote:
> In addition to comments, maybe put all 3 in a single var block?

SGTM, thanks all.

https://codereview.appspot.com/8352043/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/8352043/diff/1/cmd/juju/bootstrap.go#newcode19
cmd/juju/bootstrap.go:19: UploadTools bool
On 2013/04/04 01:53:08, thumper wrote:
> Is there a reason that Constraints, UploadTools and FakeSeries are exported?

History/consistency. If your flags/args all do predictable things to exported
fields, you can test command initialization exhaustively in terms of what gets
set, and then separately test what happens when you run hand-built commands.

We don't do this very often, necessarily, but I'm loath to drop the pattern --
one day I would like to put these things in a real package and give them clean
tests, and IMO it'll be easier to do so if we bear this in mind.

https://codereview.appspot.com/8352043/diff/1/cmd/juju/bootstrap.go#newcode34
cmd/juju/bootstrap.go:34: f.Var(seriesVar{&c.FakeSeries}, "fake-series", "clone
uploaded tools for supplied serieses")
On 2013/04/04 01:53:08, thumper wrote:
> I have to admit that the seriesVar bit made this all look a bit too magical.

I don't think this is magical... this is what you're meant to do to parse args
that aren't straight-up StringVar/IntVar/whatever.

> Why not just have a method turned a string into an array of strings?

Because it's absurd to have two separate copies of the same information in a
type, but making one useful and one not. IMO InitCommand should leave you with
an initialized command, holding only the data it actually needs... anything else
is an invitation to inconsistency.

https://codereview.appspot.com/8352043/diff/1/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/8352043/diff/1/cmd/juju/upgradejuju_test.go#ne...
cmd/juju/upgradejuju_test.go:173: c.Logf("\ntest %d: %s\n", i, test.about)
On 2013/04/04 08:56:38, dimitern wrote:
> On 2013/04/04 07:36:37, dfc wrote:
> > I don't think either of the \n's are needed.
> 
> +1, except probably one of them, in case you want to see the NL in the log
dump.

When you get a few failures scattered through acres of logspam, it's *really*
nice to be able to scan up and see a visually distinct marker that leads one
back to the actual test that failed. But I guess that only needs one new line
;p.

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

https://codereview.appspot.com/8352043/diff/1/environs/tools.go#newcode113
environs/tools.go:113: log.Infof("environs: built tools %v (%dkB)",
toolsVersion, (size+512)/1024)
On 2013/04/04 08:56:38, dimitern wrote:
> what's the magic 512 here?

Half of 1024; ie, round to the nearest kB rather than down.

https://codereview.appspot.com/8352043/diff/1/environs/tools_test.go
File environs/tools_test.go (right):

https://codereview.appspot.com/8352043/diff/1/environs/tools_test.go#newcode142
environs/tools_test.go:142: 
On 2013/04/04 08:56:38, dimitern wrote:
> d

Done.