Merge lp://staging/~fwereade/juju-core/environs-tools-provisioning into lp://staging/~juju/juju-core/trunk

Reviewers: mp+158780_code.launchpad.net,

Message:
Please take a look.

Description:
environs: FindInstanceTools, FindBootstrapTools

These funcs will replace use of FindTools/BestTools in (a) followup
branch(es).

https://code.launchpad.net/~fwereade/juju-core/environs-tools-provisioning/+merge/158780

Requires:
https://code.launchpad.net/~fwereade/juju-core/upgrade-select-tools/+merge/158596

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8727044/

Affected files:
   A [revision details]
   M environs/tools.go
   M environs/tools_test.go

looks really good. a couple of comments below.

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

https://codereview.appspot.com/8727044/diff/1/environs/tools.go#newcode137
environs/tools.go:137: // environment's configuration.
given the name of this function, i'm not sure that this casual
side-effect is great.

perhaps it might work better if we made the side effect the principal
purpose of the function.

something like this, perhaps?

// ChooseAgentVersion makes sure that the
// environment's agent version is set to a value
// that is compatible with the tools in the
// environment's storage, given the supplied constraints,
// setting it if necessary.
//
// It returns the non-empty set of tools with that version.
func ChooseAgentVersion(environ Envirom, con constraints.Value) (list
tools,List, err error)

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

https://codereview.appspot.com/8727044/diff/1/environs/tools_test.go#newcode780
environs/tools_test.go:780: // These should never be chosen.
how do we know that they're not?

https://codereview.appspot.com/8727044/diff/1/environs/tools_test.go#newcode785
environs/tools_test.go:785: actual, err :=
environs.FindBootstrapTools(s.env, cons)
i don't see any tests for the fact that FindBootstrapTools can set the
environment's agent version.

https://codereview.appspot.com/8727044/

LGTM

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

https://codereview.appspot.com/8727044/diff/1/environs/tools_test.go#newcode585
environs/tools_test.go:585: var findBootstrapToolsTests = []struct {
nice, exhaustive tests!

https://codereview.appspot.com/8727044/diff/1/environs/tools_test.go#newcode607
environs/tools_test.go:607: info: "released cli: cli arch
ignored",
why is it ignored (both cases below) ?

https://codereview.appspot.com/8727044/

Please take a look.

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

https://codereview.appspot.com/8727044/diff/1/environs/tools.go#newcode137
environs/tools.go:137: // environment's configuration.
On 2013/04/15 11:12:13, rog wrote:
> given the name of this function, i'm not sure that this casual
side-effect is
> great.

> perhaps it might work better if we made the side effect the principal
purpose of
> the function.

> something like this, perhaps?

> // ChooseAgentVersion makes sure that the
> // environment's agent version is set to a value
> // that is compatible with the tools in the
> // environment's storage, given the supplied constraints,
> // setting it if necessary.
> //
> // It returns the non-empty set of tools with that version.
> func ChooseAgentVersion(environ Envirom, con constraints.Value) (list
> tools,List, err error)

I'm not sure we came to any firm conclusions when we discussed this on
IRC. I'm starting to ponder putting all this logic quietly inside
Bootstrap() and passing possibleTools on down to Environ.Bootstrap()...
but for now I call progress-not-perfection.

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

https://codereview.appspot.com/8727044/diff/1/environs/tools_test.go#newcode607
environs/tools_test.go:607: info: "released cli: cli arch
ignored",
On 2013/04/15 14:45:32, dimitern wrote:
> why is it ignored (both cases below) ?

CLI arch/series should not determine arch/series in the cloud.

https://codereview.appspot.com/8727044/diff/1/environs/tools_test.go#newcode780
environs/tools_test.go:780: // These should never be chosen.
On 2013/04/15 11:12:13, rog wrote:
> how do we know that they're not?

(1) the tests demonstrably fail to find valid tools that are in vAll
(2) if they did find the wrong tools, the URLs check would catch them
anyway

https://codereview.appspot.com/8727044/diff/1/environs/tools_test.go#newcode785
environs/tools_test.go:785: actual, err :=
environs.FindBootstrapTools(s.env, cons)
On 2013/04/15 11:12:13, rog wrote:
> i don't see any tests for the fact that FindBootstrapTools can set the
> environment's agent version.

Whoops, thank you.

https://codereview.appspot.com/8727044/

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

https://codereview.appspot.com/8727044/diff/1/environs/tools.go#newcode137
environs/tools.go:137: // environment's configuration.
On 2013/04/16 02:29:56, fwereade wrote:
> On 2013/04/15 11:12:13, rog wrote:
> > given the name of this function, i'm not sure that this casual
side-effect is
> > great.
> >
> > perhaps it might work better if we made the side effect the
principal purpose
> of
> > the function.
> >
> > something like this, perhaps?
> >
> > // ChooseAgentVersion makes sure that the
> > // environment's agent version is set to a value
> > // that is compatible with the tools in the
> > // environment's storage, given the supplied constraints,
> > // setting it if necessary.
> > //
> > // It returns the non-empty set of tools with that version.
> > func ChooseAgentVersion(environ Envirom, con constraints.Value)
(list
> > tools,List, err error)

> I'm not sure we came to any firm conclusions when we discussed this on
IRC. I'm
> starting to ponder putting all this logic quietly inside Bootstrap()
and passing
> possibleTools on down to Environ.Bootstrap()... but for now I call
> progress-not-perfection.

please, please change the name so it expresses the fact that it has a
significant side effect. i know you're not keen on ChangeAgentVersion
(or, presumably EnsureAgentVersion or SetAgentVersion) but something
different from FindBootstrapTools would make me happier.

https://codereview.appspot.com/8727044/

https://codereview.appspot.com/8727044/

On 2013/04/16 12:25:45, rog wrote:
> presumably EnsureAgentVersion or SetAgentVersion) but something
different from
> FindBootstrapTools would make me happier.

EnsureAgentVersion is fine by me, because it's suitably ambiguous. I
think I know where to take it next regardless.

https://codereview.appspot.com/8727044/

*** Submitted:

environs: FindInstanceTools, FindBootstrapTools

These funcs will replace use of FindTools/BestTools in (a) followup
branch(es).

Except roger doesn't like FindBootstrapTools because it has side
effects, so
it's EnsureAgentVersion instead, which still has side effects and isn't
clearly about bootstrap any more and is inconsistent with all the other
tools functions. But it's Better.

R=rog, dimitern
CC=
https://codereview.appspot.com/8727044

https://codereview.appspot.com/8727044/