Refactoring environ mongo functions and version.

Refactoring environ mongo functions and version.

This branch started as one to move the Mongo tool selection functions out of
environs/tools.go and into their own file.  I also changed the parameters to
be only what the function actually needed.  This made the call-sites simpler
too.

Then I added a few more helper functions to the versions package, and started
using them in locations in the code.  Too many places were using
version.Current when what they actually wanted was a string representation of
the version number, or the default series.  Additional methods were added for
CurrentSeries, and CurrentArch, so they could be exposed independently of
version.Current.

A DefaultSeries method is added to express the intent that by default we
support precise, and have this different to version.Current, or
version.CurrentSeries, which represents the current machine that the code is
running on.

Putting fake tools was modified in dummy to put both the current series, and
the default series if it is different to the current series.  Same for the
other putFakeTools functions that were used by ec2 and openstack.  That method
was extracted into a common testing package, but had to be in a new package as
there were circular dependencies trying to use the main testing module due to
internal test in environs.

https://code.launchpad.net/~thumper/juju-core/mongo-driveby/+merge/156992

(do not edit description out of merge proposal)

[thumper, 2013-04-03 22:36:16]

Please take a look.

[fwereade, 2013-04-03 22:56:03]

The core of this is great but I have a bunch of questions and suggestions:

https://codereview.appspot.com/8348043/diff/1/environs/config/config_test.go
File environs/config/config_test.go (right):

https://codereview.appspot.com/8348043/diff/1/environs/config/config_test.go#...
environs/config/config_test.go:522: 
Surely, drop default-series here...

https://codereview.appspot.com/8348043/diff/1/environs/config/config_test.go#...
environs/config/config_test.go:534: 
...and add it here? So we can test that DefaultSeries is poked in by default?

https://codereview.appspot.com/8348043/diff/1/environs/mongo.go
File environs/mongo.go (right):

https://codereview.appspot.com/8348043/diff/1/environs/mongo.go#newcode22
environs/mongo.go:22: url = fmt.Sprintf("http://juju-dist.s3.amazonaws.com/%s",
path)
This no-error-return is still crack. Please at least check that the file exists
with a HEAD request or something.

https://codereview.appspot.com/8348043/diff/1/environs/openstack/live_test.go
File environs/openstack/live_test.go (right):

https://codereview.appspot.com/8348043/diff/1/environs/openstack/live_test.go...
environs/openstack/live_test.go:112: 
I approve of this... but not so much of magical tool-putting in these tests.
Might be outside scope... but, well, do we really want/need to do this? How many
tests are only for the dummy env?

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

https://codereview.appspot.com/8348043/diff/1/environs/testing/tools.go#newco...
environs/testing/tools.go:22: toolsVersion.Series = version.DefaultSeries()
Any reason we couldn't get this series from the environ config?

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

https://codereview.appspot.com/8348043/diff/1/environs/tools_test.go#newcode225
environs/tools_test.go:225: 
This feels off... not actually crazy, but only by coincidence, if you like. That
is, it *will* be this value... but only because the source of the running tests
will be the same as the source from which the tools were built.

But, fair enough, it is less crazy than before, I think.

https://codereview.appspot.com/8348043/diff/1/version/version.go
File version/version.go (right):

https://codereview.appspot.com/8348043/diff/1/version/version.go#newcode23
version/version.go:23: 
Haven't seen many uses of this... can we drop it maybe?

https://codereview.appspot.com/8348043/diff/1/version/version.go#newcode28
version/version.go:28: 
I think version.CurrentNumber() reads more nicely...

https://codereview.appspot.com/8348043/diff/1/version/version.go#newcode37
version/version.go:37: 
I don't *think* this belongs here... environs/config seems more natural to me.
Are there lots of other non-test users of it I've missed?

https://codereview.appspot.com/8348043/diff/1/version/version.go#newcode51
version/version.go:51: 
Yeah, CurrentNumber would definitely look nicer here, at least...

[thumper, 2013-04-04 01:04:37]

https://codereview.appspot.com/8348043/diff/1/environs/config/config_test.go
File environs/config/config_test.go (right):

https://codereview.appspot.com/8348043/diff/1/environs/config/config_test.go#...
environs/config/config_test.go:522: 
On 2013/04/03 22:56:04, fwereade wrote:
> Surely, drop default-series here...

Done.

https://codereview.appspot.com/8348043/diff/1/environs/config/config_test.go#...
environs/config/config_test.go:534: 
On 2013/04/03 22:56:04, fwereade wrote:
> ...and add it here? So we can test that DefaultSeries is poked in by default?

yes, certainly.  This was me being too mechanical...

https://codereview.appspot.com/8348043/diff/1/environs/mongo.go
File environs/mongo.go (right):

https://codereview.appspot.com/8348043/diff/1/environs/mongo.go#newcode22
environs/mongo.go:22: url = fmt.Sprintf("http://juju-dist.s3.amazonaws.com/%s",
path)
On 2013/04/03 22:56:04, fwereade wrote:
> This no-error-return is still crack. Please at least check that the file
exists
> with a HEAD request or something.

I do agree in general, but somewhat outside the scope of this branch.  I have
though filed a bug, and left a comment pointing to the bug.  It is possible that
with our change to the way that we get mongo delivered to the machines, that
this will all go away.

https://codereview.appspot.com/8348043/diff/1/environs/openstack/live_test.go
File environs/openstack/live_test.go (right):

https://codereview.appspot.com/8348043/diff/1/environs/openstack/live_test.go...
environs/openstack/live_test.go:112: 
On 2013/04/03 22:56:04, fwereade wrote:
> I approve of this... but not so much of magical tool-putting in these tests.
> Might be outside scope... but, well, do we really want/need to do this? How
many
> tests are only for the dummy env?

This change was a little mechanical, and not a lot of thought went into whether
or not the test should be doing this, however without it, there are failures, so
it was easier to duplicate the behaviour than reengineer the tests.

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

https://codereview.appspot.com/8348043/diff/1/environs/testing/tools.go#newco...
environs/testing/tools.go:22: toolsVersion.Series = version.DefaultSeries()
On 2013/04/03 22:56:04, fwereade wrote:
> Any reason we couldn't get this series from the environ config?

Possibly... We don't have access to the environ config here though.  I think
that there are places in the code that still use Current and so I couldn't just
change the PutFakeTools to only use the DefaultSeries because then the other
places failed.  I didn't want to dive too deep on the refactoring, but I agree
in principle that we should be more controlled tools get put and when.  But not
right now.

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

https://codereview.appspot.com/8348043/diff/1/environs/tools_test.go#newcode225
environs/tools_test.go:225: 
On 2013/04/03 22:56:04, fwereade wrote:
> This feels off... not actually crazy, but only by coincidence, if you like.
That
> is, it *will* be this value... but only because the source of the running
tests
> will be the same as the source from which the tools were built.
> 
> But, fair enough, it is less crazy than before, I think.

I agree entirely.  I feel that there is quite a lot of tests that work in
practice but aren't well enough designed to be exhaustive.

https://codereview.appspot.com/8348043/diff/1/version/version.go
File version/version.go (right):

https://codereview.appspot.com/8348043/diff/1/version/version.go#newcode23
version/version.go:23: 
On 2013/04/03 22:56:04, fwereade wrote:
> Haven't seen many uses of this... can we drop it maybe?

Done.

https://codereview.appspot.com/8348043/diff/1/version/version.go#newcode28
version/version.go:28: 
On 2013/04/03 22:56:04, fwereade wrote:
> I think version.CurrentNumber() reads more nicely...

Done.

https://codereview.appspot.com/8348043/diff/1/version/version.go#newcode37
version/version.go:37: 
On 2013/04/03 22:56:04, fwereade wrote:
> I don't *think* this belongs here... environs/config seems more natural to me.
> Are there lots of other non-test users of it I've missed?

Moved version.DefaultSeries() to environs/config DefaultSeries().

https://codereview.appspot.com/8348043/diff/1/version/version.go#newcode51
version/version.go:51: 
On 2013/04/03 22:56:04, fwereade wrote:
> Yeah, CurrentNumber would definitely look nicer here, at least...

Done.

[thumper, 2013-04-04 01:07:39]

Please take a look.

[fwereade, 2013-04-04 07:02:24]

LGTM, this is a clear improvement. Thanks.

https://codereview.appspot.com/8348043/diff/17001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/8348043/diff/17001/environs/ec2/ec2.go#newcode288
environs/ec2/ec2.go:288: mongoURL := environs.MongoURL(e, tools.Series,
tools.Arch)
Hmm, just a thought here... unless we can be sure we'll be using the same
version of mongo always and forever, I think we're discarding relevant
information.

[dfc, 2013-04-04 07:14:43]

Didn't read the whole thing. I couldn't find the definition of
version.CurrentNumber.

https://codereview.appspot.com/8348043/diff/17001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/8348043/diff/17001/environs/config/config.go#n...
environs/config/config.go:31: func DefaultSeries() string {
Why not make this a constant ?

[thumper, 2013-04-04 07:43:52]

https://codereview.appspot.com/8348043/diff/17001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/8348043/diff/17001/environs/ec2/ec2.go#newcode288
environs/ec2/ec2.go:288: mongoURL := environs.MongoURL(e, tools.Series,
tools.Arch)
On 2013/04/04 07:02:25, fwereade wrote:
> Hmm, just a thought here... unless we can be sure we'll be using the same
> version of mongo always and forever, I think we're discarding relevant
> information.

No we are not.  The Number part of the version.Binary was never used.

[thumper, 2013-04-04 07:44:31]

On 2013/04/04 07:14:43, dfc wrote:
> Didn't read the whole thing. I couldn't find the definition of
> version.CurrentNumber.
> 
> https://codereview.appspot.com/8348043/diff/17001/environs/config/config.go
> File environs/config/config.go (right):
> 
>
https://codereview.appspot.com/8348043/diff/17001/environs/config/config.go#n...
> environs/config/config.go:31: func DefaultSeries() string {
> Why not make this a constant ?

Because then it wouldn't be a string.

[dfc, 2013-04-04 07:46:43]

>> Why not make this a constant ?
>
>
> Because then it wouldn't be a string.

It would be a constant string

[thumper, 2013-04-04 08:51:56]

Please take a look.

[dfc, 2013-04-04 08:57:22]

LGTM. Thank you.

https://codereview.appspot.com/8348043/diff/16002/version/version.go
File version/version.go (right):

https://codereview.appspot.com/8348043/diff/16002/version/version.go#newcode43
version/version.go:43: Arch:   CurrentArch(),
sooo much better!

[thumper, 2013-04-04 09:40:12]

*** Submitted:

Refactoring environ mongo functions and version.

This branch started as one to move the Mongo tool selection functions out of
environs/tools.go and into their own file.  I also changed the parameters to
be only what the function actually needed.  This made the call-sites simpler
too.

Then I added a few more helper functions to the versions package, and started
using them in locations in the code.  Too many places were using
version.Current when what they actually wanted was a string representation of
the version number, or the default series.  Additional methods were added for
CurrentSeries, and CurrentArch, so they could be exposed independently of
version.Current.

A DefaultSeries method is added to express the intent that by default we
support precise, and have this different to version.Current, or
version.CurrentSeries, which represents the current machine that the code is
running on.

Putting fake tools was modified in dummy to put both the current series, and
the default series if it is different to the current series.  Same for the
other putFakeTools functions that were used by ec2 and openstack.  That method
was extracted into a common testing package, but had to be in a new package as
there were circular dependencies trying to use the main testing module due to
internal test in environs.

R=fwereade, dfc
CC=
https://codereview.appspot.com/8348043

[rog, 2013-09-20 17:45:54]

On 2013/04/04 09:40:12, thumper wrote:
> Additional methods were added for
> CurrentSeries, and CurrentArch, so they could be exposed independently of
> version.Current.

Does anyone remember why this was considered a good thing?

Having two sources of truth for the current series seems
potentially risky - I saw a test problem reported recently building
under Saucy that I think might be caused by this, though
I can't be sure.