In general I like that responsibility for setting up tools has been
delegated to the providers. My main concern is that, as implemented, the
bootstrap is not exiting early enough if the environment is already
bootstrapped.


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

https://codereview.appspot.com/14433058/diff/9001/cmd/juju/bootstrap.go#newcode100
cmd/juju/bootstrap.go:100:
One implication of removing this check and running it as part of
bootstrap.Bootstrap() below is that we may well end up uploading tools
only to then tell the user "sorry, already bootstrapped". This check
really does need to be run asap in the bootstrap process, before we do
anything, so that we fail as early as possible.

https://codereview.appspot.com/14433058/diff/9001/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/9001/environs/bootstrap/bootstrap.go#newcode50
environs/bootstrap/bootstrap.go:50: // and update the agent-version
configuration attribute.
s/update/updates

https://codereview.appspot.com/14433058/diff/9001/environs/bootstrap/bootstrap.go#newcode51
environs/bootstrap/bootstrap.go:51: func SelectBootstrapTools(environ
environs.Environ, toolsList coretools.List) (coretools.List, error) {
could we rename toolsList to availableTools or possibleTools to better
reflect the purpose of the parameter? callers of this method tend to
pass in a possibleTools variable so maybe that is the name to use

https://codereview.appspot.com/14433058/diff/9001/environs/manual/fakessh.go
File environs/manual/fakessh.go (right):

https://codereview.appspot.com/14433058/diff/9001/environs/manual/fakessh.go#newcode1
environs/manual/fakessh.go:1: // Copyright 2013 Canonical Ltd.
Why did this file get renamed from fakessh_test.go?
The contents are only for testing. In general, we only use non
"_test.go" files for testing if there are methods called from outside
the package (I don't think that's the case here?); and if we do, we
usually introduce a "testing" package into which such files are put to
make it clear that the contents are for testing and not production
business logic.

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

https://codereview.appspot.com/14433058/diff/9001/environs/testing/tools.go#newcode42
environs/testing/tools.go:42: sync.DefaultToolsLocation = c.MkDir() //
stop sync from going to s3
\o/

https://codereview.appspot.com/14433058/diff/9001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/14433058/diff/9001/provider/common/bootstrap.go#newcode29
provider/common/bootstrap.go:29: // before potentially uploading any
tools.
Trouble is, we may already have uploaded tools prior to getting to this
point.

https://codereview.appspot.com/14433058/diff/9001/provider/common/bootstrap.go#newcode76
provider/common/bootstrap.go:76: func SetBootstrapTools(env
environs.Environ, series string, arch *string) (coretools.List, error) {
bikeshed - perhaps call this either SetupBootstrapTools or
EnsureBootstrapTools. I think I prefer the latter.

https://codereview.appspot.com/14433058/