Add series to StartInstance, and remove the tools

Add series to StartInstance, and remove the tools

In order to facilitate some simpler testing, a helper method was added for
StartInstance.

An unknown series for the dummy provider now raises a NotFoundError saying no
tools, as this is the behaviour that is now happening with the ec2 and
openstack providers.

The series is passed through to the private start instance for the providers
where it is used to find tools, and is also passed into the findInstanceSpec.

https://code.launchpad.net/~thumper/juju-core/start-instance-series/+merge/154635

(do not edit description out of merge proposal)

[thumper, 2013-03-21 09:31:37]

Please take a look.

[jameinel, 2013-03-21 10:00:28]

LGTM

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

https://codereview.appspot.com/7943043/diff/1/environs/interface.go#newcode175
environs/interface.go:175: StartInstance(machineId string, series string, cons
constraints.Value, info *state.Info, apiInfo *api.Info) (Instance, error)
I'm a little concerned that we're going off the deep end a bit on adding
parameters to this function. It certainly seems like we are at a point we should
have a stateful struct. (We already have 5 params, will there be more?)

In the immediate term, I can certainly live with this, though.

[fwereade, 2013-03-21 10:14:06]

LGTM on the assumption that there's a bit more coming (see my comment in
ec2.go).

https://codereview.appspot.com/7943043/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/7943043/diff/1/environs/ec2/ec2.go#newcode441
environs/ec2/ec2.go:441: v := version.Current
Ideally, at this point, tools should always be nil; it might even be
panic-worthy if they're not. Is this not yet the case, or did I miss something?

I'm pretty sure we should be taking Number from the env config's agent-version,
and dropping the HighestVersion flag. Are these explicitly planned at the
moment?

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

https://codereview.appspot.com/7943043/diff/1/environs/interface.go#newcode175
environs/interface.go:175: StartInstance(machineId string, series string, cons
constraints.Value, info *state.Info, apiInfo *api.Info) (Instance, error)
On 2013/03/21 10:00:28, jameinel wrote:
> I'm a little concerned that we're going off the deep end a bit on adding
> parameters to this function. It certainly seems like we are at a point we
should
> have a stateful struct. (We already have 5 params, will there be more?)
> 
> In the immediate term, I can certainly live with this, though.

The info params are kinda wrong -- if anything ought to know how to connect to
the environment, it's the environment, and it probably shouldn't have to be
told. If we got this down to id/series/constraints I would be happy.

Not saying there won't ever be more, but I don't anticipate any at the moment.

[thumper, 2013-03-21 20:39:49]

https://codereview.appspot.com/7943043/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/7943043/diff/1/environs/ec2/ec2.go#newcode441
environs/ec2/ec2.go:441: v := version.Current
On 2013/03/21 10:14:07, fwereade wrote:
> Ideally, at this point, tools should always be nil; it might even be
> panic-worthy if they're not. Is this not yet the case, or did I miss
something?
> 
> I'm pretty sure we should be taking Number from the env config's
agent-version,
> and dropping the HighestVersion flag. Are these explicitly planned at the
> moment?

If coming through the boostrap route, the tools is set already.

I explicitly didn't change the tool selection criteria at this stage as it was
really outside the scope of this change.  However, we should probably still fix
it.

If selecting compat version and using the agent version is the right thing to
do, we could change it here, but how would we test it?

[thumper, 2013-03-21 20:49:18]

*** Submitted:

Add series to StartInstance, and remove the tools

In order to facilitate some simpler testing, a helper method was added for
StartInstance.

An unknown series for the dummy provider now raises a NotFoundError saying no
tools, as this is the behaviour that is now happening with the ec2 and
openstack providers.

The series is passed through to the private start instance for the providers
where it is used to find tools, and is also passed into the findInstanceSpec.

R=jameinel, fwereade
CC=
https://codereview.appspot.com/7943043