environs: StartInstance accepts constraints

environs: StartInstance accepts constraints

...and in ec2 honours them.

https://code.launchpad.net/~fwereade/juju-core/bootstrap-constraints-6/+merge/153862

Requires: https://code.launchpad.net/~fwereade/juju-core/bootstrap-constraints-5/+merge/153788

(do not edit description out of merge proposal)

[fwereade, 2013-03-18 16:23:22]

Please take a look.

[thumper, 2013-03-19 01:36:40]

LGTM

Part of me thinks we should keep track of things that should be refactored
later.  Part of me thinks we'll find enough things that we don't need to worry.

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

https://codereview.appspot.com/7735045/diff/1/environs/ec2/local_test.go#newc...
environs/ec2/local_test.go:252: bsstate, err := ec2.LoadState(t.env)
Personally I'd prefer bootstrapState, but I don't mind typing.  At least you
didn't call it 's'.

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

https://codereview.appspot.com/7735045/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:104: inst, err := t.Env.StartInstance("0",
state.Constraints{}, testing.InvalidStateInfo("0"), testing.InvalidAPIInfo("0"),
nil)
I see the same change in six places in this file and immediately think "this
needs to be factored out", but not now.

[jameinel, 2013-03-19 08:26:56]

LGTM

Though your overview says that ec2 honors the contstraints, but I don't see any
actual change to ec2 code other than supplying the constraints to
startInstanceParams.

I guess that work was already done, and you are comparing 'm1.medium' to
'm1.small' as the default. I wonder if that is a bit too implicit (it was hard
for me to figure out how you were telling that the constraint was honored.)

func (t *LiveTests) TestInstanceAttributes(c *C) {

Could you instead have a separate test about TestConstraints and does the sort
of "without constraints I get X, with constraints I get Y" ?

You don't have to, but it would be clearer to me that you were actually testing
constraints have an effect.

[dimitern, 2013-03-19 09:26:43]

LGTM, but I'd suggest to change the argument to *state.Constraints, so nil can
be passed to save some writing.

[fwereade, 2013-03-20 11:53:27]

On 2013/03/19 08:26:56, jameinel wrote:
> LGTM
> 
> Though your overview says that ec2 honors the contstraints, but I don't see
any
> actual change to ec2 code other than supplying the constraints to
> startInstanceParams.
> 
> I guess that work was already done, and you are comparing 'm1.medium' to
> 'm1.small' as the default. I wonder if that is a bit too implicit (it was hard
> for me to figure out how you were telling that the constraint was honored.)
> 
> func (t *LiveTests) TestInstanceAttributes(c *C) {
> 
> Could you instead have a separate test about TestConstraints and does the sort
> of "without constraints I get X, with constraints I get Y" ?
> 
> You don't have to, but it would be clearer to me that you were actually
testing
> constraints have an effect.

The main reason for adapting the existing test was to avoid launching two
instances for two separate tests, but I guess I really need two instances
anyway. So I'll make that test just check the result of the default constraints
(alongside the dns name check), and add another to just check the effect of
specifying constraints. Cheers.

[fwereade, 2013-03-20 11:55:16]

On 2013/03/19 08:26:56, jameinel wrote:
> LGTM
> 
> Though your overview says that ec2 honors the contstraints, but I don't see
any
> actual change to ec2 code other than supplying the constraints to
> startInstanceParams.
> 
> I guess that work was already done, and you are comparing 'm1.medium' to
> 'm1.small' as the default. I wonder if that is a bit too implicit (it was hard
> for me to figure out how you were telling that the constraint was honored.)
> 
> func (t *LiveTests) TestInstanceAttributes(c *C) {
> 
> Could you instead have a separate test about TestConstraints and does the sort
> of "without constraints I get X, with constraints I get Y" ?
> 
> You don't have to, but it would be clearer to me that you were actually
testing
> constraints have an effect.

The main reason for adapting the existing test was to avoid launching two
instances for two separate tests, but I guess I really need two instances
anyway. So I'll make that test just check the result of the default constraints
(alongside the dns name check), and add another to just check the effect of
specifying constraints. Cheers.

[fwereade, 2013-03-20 11:56:03]

On 2013/03/19 09:26:43, dimitern wrote:
> LGTM, but I'd suggest to change the argument to *state.Constraints, so nil can
> be passed to save some writing.

I don't think it's worth muddying the type's intended usage just to save some
typing in tests. As thumper points out, it's the repeated calls that should be
refactored anyway.

[fwereade, 2013-03-20 13:34:37]

*** Submitted:

environs: StartInstance accepts constraints

...and in ec2 honours them.

R=
CC=
https://codereview.appspot.com/7735045

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

https://codereview.appspot.com/7735045/diff/1/environs/ec2/local_test.go#newc...
environs/ec2/local_test.go:252: bsstate, err := ec2.LoadState(t.env)
On 2013/03/19 01:36:40, thumper wrote:
> Personally I'd prefer bootstrapState, but I don't mind typing.  At least you
> didn't call it 's'.

Done.

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

https://codereview.appspot.com/7735045/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:104: inst, err := t.Env.StartInstance("0",
state.Constraints{}, testing.InvalidStateInfo("0"), testing.InvalidAPIInfo("0"),
nil)
On 2013/03/19 01:36:40, thumper wrote:
> I see the same change in six places in this file and immediately think "this
> needs to be factored out", but not now.

Agreed on both points.