Code review comment for lp://staging/~fwereade/juju-core/environs-tools-provisioning

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/