Code review comment for lp:~dimitern/goamz/vpc-instance-addons

Please take a look.

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2.go#newcode299
ec2/ec2.go:299: // given, a VPC-enabled instances will be started.
Cannot specify
On 2014/02/06 14:56:14, rog wrote:
> s/instances/instance/

> But this whole paragraph is a bit hard to read.
> Can't we just leave it out and let the user
> get the gory details from the amz docs, like
> most of the other params?

Done.

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go#newcode141
ec2/ec2t_test.go:141: c.Check(err, IsNil, Commentf("%d INSTANCES LEFT
RUNNING!!!", len(ids)))
On 2014/02/06 14:56:14, rog wrote:
> Do we really need to fail the test if we can't terminate the instances
> immediately?

Yes we do! We depend on TerminateInstances being successful. If it
returns an error there's no point in the loop below. I even changed it
to Assert, so this more obvious.

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go#newcode164
ec2/ec2t_test.go:164: idsLeft[inst.InstanceId] = false
On 2014/02/06 14:56:14, rog wrote:
> if you did:

> delete(idsLeft, inst.InstanceId)

> you wouldn't need the

> if left {

> test in the range loop below.

Done.

https://codereview.appspot.com/60620043/diff/20001/ec2/ec2t_test.go#newcode653
ec2/ec2t_test.go:653: // some of the reset depend on it, so they can't
be deleted before
On 2014/02/06 14:56:14, rog wrote:
> s/the some of the reset/some of the rest/ ?

> I don't understand how the comment applies to the statement
> though - g[0] is first in the deleteGroups call, not g[3].

By "first" I meant "first of the set g[3], g[1], g[2]". I'll rephrase.

https://codereview.appspot.com/60620043/