environs: FindInstanceTools, FindBootstrapTools

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.

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)

[fwereade, 2013-04-14 11:12:30]

Please take a look.

[rog, 2013-04-15 11:12:12]

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.

[dimitern, 2013-04-15 14:45:32]

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) ?

[fwereade, 2013-04-16 02:29:56]

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.

[rog, 2013-04-16 12:25:45]

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.

[fwereade, 2013-04-16 13:30:32]

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.

[fwereade, 2013-04-17 05:18:45]

*** 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