Extract common bootstrap code / tools search

Extract common bootstrap code / tools search

This branch reduces the multiple call points for FindBootstrapTools
and FindInstanceTools. In so doing, bug 1199847 was addressed - common
code was extracted for starting instances and bootstrapping environs.
This means that we no longer have gobfuls of code duplication in each
provider and I can now easily replace the current tools finding logic
with simplestreams stuff.

https://code.launchpad.net/~wallyworld/juju-core/simplify-tools-search/+merge/181491

Requires: https://code.launchpad.net/~wallyworld/juju-core/refactor-tools-packaging/+merge/181186

(do not edit description out of merge proposal)

[wallyworld, 2013-08-22 08:48:31]

Please take a look.

[jameinel, 2013-08-22 09:59:42]

I really like to see the code getting shared. And a lot of this patch just looks
like the normal upheaval from moving code around.

I highlighted a couple of areas that I'd like to get feedback on, but it is
looking good so far.

https://codereview.appspot.com/13081044/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13081044/diff/1/environs/bootstrap/bootstrap.g...
environs/bootstrap/bootstrap.go:59: log.Infof("bootstrapping environment %q",
environ.Name())
you added a logger, I would imagine you want to call:
logger.Infof
rather than
log.Infof

I'm a bit surprised that you import "juju-core/log" at all in this file, as I
thought we were getting away from that package.

https://codereview.appspot.com/13081044/diff/1/provider/instance.go
File provider/instance.go (right):

https://codereview.appspot.com/13081044/diff/1/provider/instance.go#newcode17
provider/instance.go:17: coretools "launchpad.net/juju-core/tools"
Of envtools vs coretools, I would probably have picked 'coretools' as the one
that would just be 'tools'. Though my experience with the "testing" packages is
that we should just rename both of them.

So "

envtools "launchpad.net/juju-core/environs/tools"
coretools "launchpad.net/juju-core/tools"

That way you never get confused in the code itself which one it is this time.

https://codereview.appspot.com/13081044/diff/1/provider/instance.go#newcode23
provider/instance.go:23: func StartInstance(broker environs.Broker, machineId,
machineNonce string, series string, cons constraints.Value,
Given we now have the term "Broker", should we be using it in preference to
"provider/" package?

https://codereview.appspot.com/13081044/diff/1/provider/instance.go#newcode44
provider/instance.go:44: 
I'm guessing this is mostly just moved code. Are there logic things that need to
be thought through carefully?

https://codereview.appspot.com/13081044/diff/1/tools/list_test.go
File tools/list_test.go (left):

https://codereview.appspot.com/13081044/diff/1/tools/list_test.go#oldcode76
tools/list_test.go:76: }
Shouldn't the test name be changed to "TestAllSeries", and aren't we missing a
TestOneSeries test?

https://codereview.appspot.com/13081044/diff/1/worker/provisioner/lxc-broker.go
File worker/provisioner/lxc-broker.go (right):

https://codereview.appspot.com/13081044/diff/1/worker/provisioner/lxc-broker....
worker/provisioner/lxc-broker.go:24: var _ tools.HasTools = (*lxcBroker)(nil)
Given normal syntax, this looks like it should be a:
var _ tools.HasTooler

since it is an interface description.

You don't need to change it here, though.

[fwereade, 2013-08-22 11:05:46]

Thanks, this is great. LGTM; trivials only.

Please make sure it still works live, though ;).

https://codereview.appspot.com/13081044/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/13081044/diff/1/environs/bootstrap/bootstrap.g...
environs/bootstrap/bootstrap.go:59: log.Infof("bootstrapping environment %q",
environ.Name())
On 2013/08/22 09:59:42, jameinel wrote:
> you added a logger, I would imagine you want to call:
> logger.Infof
> rather than
> log.Infof
> 
> I'm a bit surprised that you import "juju-core/log" at all in this file, as I
> thought we were getting away from that package.

+1

https://codereview.appspot.com/13081044/diff/1/environs/broker.go
File environs/broker.go (right):

https://codereview.appspot.com/13081044/diff/1/environs/broker.go#newcode22
environs/broker.go:22: machineConfig *cloudinit.MachineConfig,
heh, is there a cycle around the cloudinit package? if so, given that we hope to
move it at some stage, it'd be nice to have a "TODO move Broker to instance
package"

https://codereview.appspot.com/13081044/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/13081044/diff/1/environs/interface.go#newcode199
environs/interface.go:199: type List tools.List
slight raised eyebrow here

https://codereview.appspot.com/13081044/diff/1/provider/instance.go
File provider/instance.go (right):

https://codereview.appspot.com/13081044/diff/1/provider/instance.go#newcode17
provider/instance.go:17: coretools "launchpad.net/juju-core/tools"
On 2013/08/22 09:59:42, jameinel wrote:
> Of envtools vs coretools, I would probably have picked 'coretools' as the one
> that would just be 'tools'. Though my experience with the "testing" packages
is
> that we should just rename both of them.
> 
> So "
> 
> envtools "launchpad.net/juju-core/environs/tools"
> coretools "launchpad.net/juju-core/tools"
> 
> That way you never get confused in the code itself which one it is this time.

+1 also. I'd like it most if that were consolidated in its own CL though.

https://codereview.appspot.com/13081044/diff/1/provider/instance.go#newcode73
provider/instance.go:73: if err2 != nil {
I do kinda prefer reusing err in its own tighter scope, but mileage reasonably
varies I guess :).

https://codereview.appspot.com/13081044/diff/1/tools/list_test.go
File tools/list_test.go (left):

https://codereview.appspot.com/13081044/diff/1/tools/list_test.go#oldcode76
tools/list_test.go:76: }
On 2013/08/22 09:59:42, jameinel wrote:
> Shouldn't the test name be changed to "TestAllSeries", and aren't we missing a
> TestOneSeries test?

+1

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

https://codereview.appspot.com/13081044/diff/1/tools/tools.go#newcode21
tools/tools.go:21: type HasTools interface {
slight raised eyebrow, but I guess there's also something nice about
`foo.(HasTools)`... let's run with it and see if it turns out to be a pain.