environs: add ListTools function and related refactoring.

environs: add ListTools function and related refactoring.

With the proposed upgrade scheme, we will want to explicitly
ask machines to run a particular version of the juju tools.
That means that the upgrader needs to be able to decide for
itself which tools are appropriate, so we factor out ListTools
so it doesn't have to list the storage for each agent.

As a knock-on effect, Environ.StartInstance now takes
the tools to start.

https://code.launchpad.net/~rogpeppe/juju-core/environs-list-tools/+merge/117305

(do not edit description out of merge proposal)

[rog, 2012-07-30 17:26:54]

Please take a look.

[rog, 2012-07-31 08:52:32]

Please take a look.

[fwereade, 2012-07-31 10:16:42]

LGTM notwithstanding discomfort over additional arg to StartInstance.

https://codereview.appspot.com/6449062/diff/5002/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6449062/diff/5002/environs/dummy/environs.go#n...
environs/dummy/environs.go:407: func (e *environ) StartInstance(machineId int,
info *state.Info, _ *environs.Tools) (environs.Instance, error) {
I feel like machineId and the tools probably deserve a struct, but maybe it's
just me thinking "we *will* need a MachineData type".

[niemeyer, 2012-07-31 18:35:06]

Huge step forward. A few trivials, but mainly concerned about testing below.

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/ec2.go#newcode310
environs/ec2/ec2.go:310: arch:   tools.Arch,
If you revert the change to the two lines above, does anything break? If it
doesn't, we have an important missing test to newly added functionality. We can
now setup the machine type by twisting the tools version.

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/live_test.go
File environs/ec2/live_test.go (right):

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:101: inst, err := t.Env.StartInstance(30,
jujutest.InvalidStateInfo, nil)
Is this ever not nil in tests?

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/storage.go
File environs/ec2/storage.go (right):

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/storage.go#newc...
environs/ec2/storage.go:89: // If the bucket is not found, it's not an error
Looks like a bug has been fixed. Do we have a test to avoid regressions?

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/storage.go#newc...
environs/ec2/storage.go:143: func maybeNotFound(err error) error {
Unused?

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go#newcode31
environs/tools.go:31: func ListTools(store StorageReader, majorVersion int)
([]*Tools, error) {
s/store/storage/

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go#newcode187
environs/tools.go:187: t := t
d

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go#newcode242
environs/tools.go:242: vers.Arch)
That fits easily in a single line.

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go#newcode246
environs/tools.go:246: // with the given version from the given environment.
This could do with some reidentation. The above one too.

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go#newcode251
environs/tools.go:251: // it gets precedence over anything in its PublicStorage.
This seems relevant for the function comment above.

https://codereview.appspot.com/6449062/diff/5002/environs/tools_test.go
File environs/tools_test.go (right):

https://codereview.appspot.com/6449062/diff/5002/environs/tools_test.go#newco...
environs/tools_test.go:334: })
Checking for 4 would be good too.

https://codereview.appspot.com/6449062/diff/5002/environs/tools_test.go#newco...
environs/tools_test.go:399: return "http://" + name, nil
Would you mind to use "<base>" rather than "http://"? Would make tests above
slightly more clear.

[rog, 2012-08-01 07:33:31]

Please take a look.

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/ec2.go#newcode310
environs/ec2/ec2.go:310: arch:   tools.Arch,
On 2012/07/31 18:35:06, niemeyer wrote:
> If you revert the change to the two lines above, does anything break? If it
> doesn't, we have an important missing test to newly added functionality. We
can
> now setup the machine type by twisting the tools version.

good catch. i've added a test that starting on an unknown architecture fails.
i'm not sure of a good way to test that the *correct* instance is chosen in a
platform-agnostic way, but findInstance is tested, so i think it's ok.

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/storage.go
File environs/ec2/storage.go (right):

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/storage.go#newc...
environs/ec2/storage.go:89: // If the bucket is not found, it's not an error
On 2012/07/31 18:35:06, niemeyer wrote:
> Looks like a bug has been fixed. Do we have a test to avoid regressions?

I had to fix a test to allow this new behaviour, so yes.

https://codereview.appspot.com/6449062/diff/5002/environs/ec2/storage.go#newc...
environs/ec2/storage.go:143: func maybeNotFound(err error) error {
On 2012/07/31 18:35:06, niemeyer wrote:
> Unused?

line 58. i suppose it could be inlined, but i don't mind much.

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go#newcode31
environs/tools.go:31: func ListTools(store StorageReader, majorVersion int)
([]*Tools, error) {
On 2012/07/31 18:35:06, niemeyer wrote:
> s/store/storage/

Done.

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go#newcode187
environs/tools.go:187: t := t
On 2012/07/31 18:35:06, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go#newcode242
environs/tools.go:242: vers.Arch)
On 2012/07/31 18:35:06, niemeyer wrote:
> That fits easily in a single line.

Done.

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go#newcode246
environs/tools.go:246: // with the given version from the given environment.
On 2012/07/31 18:35:06, niemeyer wrote:
> This could do with some reidentation. The above one too.

Done.

https://codereview.appspot.com/6449062/diff/5002/environs/tools.go#newcode251
environs/tools.go:251: // it gets precedence over anything in its PublicStorage.
On 2012/07/31 18:35:06, niemeyer wrote:
> This seems relevant for the function comment above.

Done.

https://codereview.appspot.com/6449062/diff/5002/environs/tools_test.go
File environs/tools_test.go (right):

https://codereview.appspot.com/6449062/diff/5002/environs/tools_test.go#newco...
environs/tools_test.go:334: })
On 2012/07/31 18:35:06, niemeyer wrote:
> Checking for 4 would be good too.

Done.

https://codereview.appspot.com/6449062/diff/5002/environs/tools_test.go#newco...
environs/tools_test.go:399: return "http://" + name, nil
On 2012/07/31 18:35:06, niemeyer wrote:
> Would you mind to use "<base>" rather than "http://"? Would make tests above
> slightly more clear.

Done.

[niemeyer, 2012-08-01 15:37:41]

LGTM

https://codereview.appspot.com/6449062/diff/5005/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6449062/diff/5005/environs/jujutest/livetests....
environs/jujutest/livetests.go:297: // architectures with the "dummy" prefix as
invalid.
Sounds like a good test. I'm not entirely sure if it's good enough to catch the
positive behavior, but we can wait until something actually breaks to find out.

Just one minor: rather than a "dummy" prefix, I suggest using "unknown". The
test then becomes a bit more readable. "dummyarch" makes it sound like a good
one for testing purposes.

[rog, 2012-08-01 16:10:08]

*** Submitted:

environs: add ListTools function and related refactoring.

With the proposed upgrade scheme, we will want to explicitly
ask machines to run a particular version of the juju tools.
That means that the upgrader needs to be able to decide for
itself which tools are appropriate, so we factor out ListTools
so it doesn't have to list the storage for each agent.

As a knock-on effect, Environ.StartInstance now takes
the tools to start.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6449062