environs/ec2: launch instances in named security groups

https://code.launchpad.net/~rogpeppe/juju/go-ec2-instance-groups/+merge/88069

(do not edit description out of merge proposal)

[rog, 2012-01-10 15:11:47]

Please take a look.

[niemeyer, 2012-01-10 16:01:15]

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/config_test.go#new...
environs/ec2/config_test.go:12: // the ec2 package because it needs to access
package internals.
This isn't reading well. Simpler/readable version:

// Use local suite since this file lives in the ec2 package
// for testing internals.

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/ec2.go#newcode20
environs/ec2/ec2.go:20: EC2    *ec2.EC2
As discussed live, let's revert those back to private.

https://codereview.appspot.com/5532058/diff/1/environs/ec2/ec2.go#newcode122
environs/ec2/ec2.go:122: // setUpGroups ensures that the juju group is in the
machine launch groups.
This comment looks wrong. This function seems to ensure that two distinct
security groups exist.

https://codereview.appspot.com/5532058/diff/1/environs/ec2/ec2.go#newcode170
environs/ec2/ec2.go:170: return fmt.Errorf("cannot create machine group %q: %v",
err)
Missing a parameter.

https://codereview.appspot.com/5532058/diff/1/environs/ec2/ec2.go#newcode173
environs/ec2/ec2.go:173: return err
s/err/nil/

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/image_test.go#newc...
environs/ec2/image_test.go:17: constraint eec2.ImageConstraint
Ugh.. eec2 looks pretty bad, and the "ec2" name seems available here.

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/local_test.go#newc...
environs/ec2/local_test.go:6: "launchpad.net/goamz/ec2"
I suggest renaming this one to lpec2 instead, both because it sounds nice than
eec2 and because our local one can remain with the expected name for the local
context.

https://codereview.appspot.com/5532058/diff/1/environs/ec2/local_test.go#newc...
environs/ec2/local_test.go:118: // recreated correctly.
Where is it verifying that this is the case? I only see it creating the old
group, but there's apparently no place checking that it's *removed* and added
back.

https://codereview.appspot.com/5532058/diff/1/environs/ec2/local_test.go#newc...
environs/ec2/local_test.go:133: // fill out the security group ids
Why is the logic below necessary? Isn't the group name enough to verify that the
correct groups have been created and assigned to the proper machine?

[rog, 2012-01-11 08:49:52]

PTAL

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/config_test.go#new...
environs/ec2/config_test.go:12: // the ec2 package because it needs to access
package internals.
On 2012/01/10 16:01:15, niemeyer wrote:
> This isn't reading well. Simpler/readable version:
> 
> // Use local suite since this file lives in the ec2 package
> // for testing internals.

Done.

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/ec2.go#newcode20
environs/ec2/ec2.go:20: EC2    *ec2.EC2
On 2012/01/10 16:01:15, niemeyer wrote:
> As discussed live, let's revert those back to private.

Done.

https://codereview.appspot.com/5532058/diff/1/environs/ec2/ec2.go#newcode122
environs/ec2/ec2.go:122: // setUpGroups ensures that the juju group is in the
machine launch groups.
On 2012/01/10 16:01:15, niemeyer wrote:
> This comment looks wrong. This function seems to ensure that two distinct
> security groups exist.

this comment is taken verbatim from the python implementation. it seems accurate
to me - have you got a better suggestion.

https://codereview.appspot.com/5532058/diff/1/environs/ec2/ec2.go#newcode170
environs/ec2/ec2.go:170: return fmt.Errorf("cannot create machine group %q: %v",
err)
On 2012/01/10 16:01:15, niemeyer wrote:
> Missing a parameter.

good catch.

https://codereview.appspot.com/5532058/diff/1/environs/ec2/ec2.go#newcode173
environs/ec2/ec2.go:173: return err
On 2012/01/10 16:01:15, niemeyer wrote:
> s/err/nil/

Done.

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/local_test.go#newc...
environs/ec2/local_test.go:6: "launchpad.net/goamz/ec2"
On 2012/01/10 16:01:15, niemeyer wrote:
> I suggest renaming this one to lpec2 instead, both because it sounds nice than
> eec2 and because our local one can remain with the expected name for the local
> context. 

done. (i used "amzec2" instead of lpec2, because they're both under launchpad)

https://codereview.appspot.com/5532058/diff/1/environs/ec2/local_test.go#newc...
environs/ec2/local_test.go:118: // recreated correctly.
On 2012/01/10 16:01:15, niemeyer wrote:
> Where is it verifying that this is the case? I only see it creating the old
> group, but there's apparently no place checking that it's *removed* and added
> back.

if the machine's group exists already, it will be deleted and then recreated -
the way we check this is that the new group should have the same name but a
different group id.
hence the logic below.

https://codereview.appspot.com/5532058/diff/1/environs/ec2/local_test.go#newc...
environs/ec2/local_test.go:133: // fill out the security group ids
On 2012/01/10 16:01:15, niemeyer wrote:
> Why is the logic below necessary? Isn't the group name enough to verify that
the
> correct groups have been created and assigned to the proper machine?

see above

[niemeyer, 2012-01-11 09:06:19]

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/ec2.go#newcode122
environs/ec2/ec2.go:122: // setUpGroups ensures that the juju group is in the
machine launch groups.
On 2012/01/11 08:49:52, rog wrote:
> this comment is taken verbatim from the python implementation. it seems
accurate
> to me - have you got a better suggestion.

Please read the logic and look for the place where
it ensures that the juju group is in the machine launch
group. It's important that you understand it.

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/image_test.go#newc...
environs/ec2/image_test.go:17: constraint eec2.ImageConstraint
On 2012/01/10 16:01:15, niemeyer wrote:
> Ugh.. eec2 looks pretty bad, and the "ec2" name seems available here.

This wasn't addressed.

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/local_test.go#newc...
environs/ec2/local_test.go:6: "launchpad.net/goamz/ec2"
On 2012/01/11 08:49:52, rog wrote:
> done. (i used "amzec2" instead of lpec2, because they're both under launchpad)

Cool, that sounds good.

https://codereview.appspot.com/5532058/diff/1/environs/ec2/local_test.go#newc...
environs/ec2/local_test.go:118: // recreated correctly.
On 2012/01/11 08:49:52, rog wrote:
> if the machine's group exists already, it will be deleted and then recreated -
> the way we check this is that the new group should have the same name but a
> different group id.
> hence the logic below.

This isn't clear at all in the logic. Let's talk live about this.

[rog, 2012-01-11 10:03:25]

PTAL

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/ec2.go#newcode122
environs/ec2/ec2.go:122: // setUpGroups ensures that the juju group is in the
machine launch groups.
On 2012/01/11 09:06:20, niemeyer wrote:
> On 2012/01/11 08:49:52, rog wrote:
> > this comment is taken verbatim from the python implementation. it seems
> accurate
> > to me - have you got a better suggestion.
> 
> Please read the logic and look for the place where
> it ensures that the juju group is in the machine launch
> group. It's important that you understand it.

Done.

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/image_test.go#newc...
environs/ec2/image_test.go:17: constraint eec2.ImageConstraint
On 2012/01/11 09:06:20, niemeyer wrote:
> On 2012/01/10 16:01:15, niemeyer wrote:
> > Ugh.. eec2 looks pretty bad, and the "ec2" name seems available here.
> 
> This wasn't addressed.

Done. (i could've sworn i'd replied to all comments!)

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

https://codereview.appspot.com/5532058/diff/1/environs/ec2/local_test.go#newc...
environs/ec2/local_test.go:118: // recreated correctly.
On 2012/01/11 09:06:20, niemeyer wrote:
> On 2012/01/11 08:49:52, rog wrote:
> > if the machine's group exists already, it will be deleted and then recreated
-
> > the way we check this is that the new group should have the same name but a
> > different group id.
> > hence the logic below.
> 
> This isn't clear at all in the logic. Let's talk live about this.

Done.

[rog, 2012-01-12 11:17:35]

PTAL

[niemeyer, 2012-01-17 00:14:39]

LGTM

https://codereview.appspot.com/5532058/diff/3002/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/5532058/diff/3002/environs/ec2/ec2.go#newcode130
environs/ec2/ec2.go:130: // per machine.
Please just reindent the comment so it is properly aligned.

https://codereview.appspot.com/5532058/diff/3002/environs/ec2/ec2.go#newcode132
environs/ec2/ec2.go:132: allGroups, err := e.ec2.SecurityGroups(nil, nil)
If you create the two groups first, you can use them to filter the call here so
it only cares about the two specific groups rather than going over all of them.
The rest may remain untouched.

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

https://codereview.appspot.com/5532058/diff/3002/environs/ec2/local_test.go#n...
environs/ec2/local_test.go:10: ec2 "launchpad.net/juju/go/environs/ec2"
s/ec2 //

[rog, 2012-01-17 09:42:15]

https://codereview.appspot.com/5532058/diff/3002/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/5532058/diff/3002/environs/ec2/ec2.go#newcode130
environs/ec2/ec2.go:130: // per machine.
On 2012/01/17 00:14:40, niemeyer wrote:
> Please just reindent the comment so it is properly aligned.

Done.

https://codereview.appspot.com/5532058/diff/3002/environs/ec2/ec2.go#newcode132
environs/ec2/ec2.go:132: allGroups, err := e.ec2.SecurityGroups(nil, nil)
On 2012/01/17 00:14:40, niemeyer wrote:
> If you create the two groups first, you can use them to filter the call here
so
> it only cares about the two specific groups rather than going over all of
them.
> The rest may remain untouched.

Done.

[rog, 2012-01-17 09:51:16]

*** Submitted:

environs/ec2: launch instances in named security groups

R=niemeyer
CC=
https://codereview.appspot.com/5532058