Merge lp:~gz/goamz/instance_group_filter into lp:goamz

[Martin Packman]

ec2: support instance prefix on group filters

The EC2 API documentation allows for alternative spellings of
the group-name and group-id filters on instances. It seems only
the prefixed forms are actually supported when using default VPC
despite claims to the contrary, so add support for them to the
test server infrastructure.

https://codereview.appspot.com/14304043/

[Martin Packman (gz)]

Reviewers: mp+188935_code.launchpad.net,

Message:
Please take a look.

Description:
ec2: support instance prefix on group filters

The EC2 API documentation allows for alternative spellings of
the group-name and group-id filters on instances. It seems only
the prefixed forms are actually supported when using default VPC
despite claims to the contrary, so add support for them to the
test server infrastructure.

https://code.launchpad.net/~gz/goamz/instance_group_filter/+merge/188935

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/14304043/

Affected files (+16, -2 lines):
   A [revision details]
   M ec2/ec2t_test.go
   M ec2/ec2test/server.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: ec2/ec2t_test.go
=== modified file 'ec2/ec2t_test.go'
--- ec2/ec2t_test.go 2013-08-15 15:35:52 +0000
+++ ec2/ec2t_test.go 2013-10-02 21:34:26 +0000
@@ -385,12 +385,24 @@
     },
     resultIds: ids(0, 1),
    }, {
+ about: "check that filtering on group id with instance prefix works",
+ filters: []filterSpec{
+ {"instance.group-id", []string{group1.Id}},
+ },
+ resultIds: ids(0, 1),
+ }, {
     about: "check that filtering on group name works",
     filters: []filterSpec{
      {"group-name", []string{group1.Name}},
     },
     resultIds: ids(0, 1),
    }, {
+ about: "check that filtering on group name with instance prefix works",
+ filters: []filterSpec{
+ {"instance.group-name", []string{group1.Name}},
+ },
+ resultIds: ids(0, 1),
+ }, {
     about: "check that filtering on image id works",
     filters: []filterSpec{
      {"image-id", []string{imageId}},

Index: ec2/ec2test/server.go
=== modified file 'ec2/ec2test/server.go'
--- ec2/ec2test/server.go 2013-08-15 16:29:50 +0000
+++ ec2/ec2test/server.go 2013-10-02 21:34:26 +0000
@@ -569,14 +569,14 @@
    return value == "i386", nil
   case "instance-id":
    return inst.id() == value, nil
- case "group-id":
+ case "instance.group-id", "group-id":
    for _, g := range inst.reservation.groups {
     if g.id == value {
      return true, nil
     }
    }
    return false, nil
- case "group-name":
+ case "instance.group-name", "group-name":
    for _, g := range inst.reservation.groups {
     if g.name == value {
      return true, nil

[Roger Peppe (rogpeppe)]

LGTM assuming the live tests pass.

I'm not sure what we should do about testing against default-VPC ec2
environments.

https://codereview.appspot.com/14304043/diff/1/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/14304043/diff/1/ec2/ec2t_test.go#newcode384
ec2/ec2t_test.go:384: {"group-id", []string{group1.Id}},
presumably this test will fail when run live against a default-VPC ec2
environment?

https://codereview.appspot.com/14304043/

[Martin Packman (gz)]

On 2013/10/02 22:18:52, rog wrote:
> LGTM assuming the live tests pass.

$ (cd ec2&&go test -amazon)
OK: 46 passed
PASS
ok launchpad.net/goamz/ec2 36.933s

> I'm not sure what we should do about testing against default-VPC ec2
> environments.

Having an easy region switch would be a good start I think.

https://codereview.appspot.com/14304043/diff/1/ec2/ec2t_test.go#newcode384
> ec2/ec2t_test.go:384: {"group-id", []string{group1.Id}},
> presumably this test will fail when run live against a default-VPC ec2
> environment?

I'm not certain that's the case, but it's what I observed in a more
complex case.

https://codereview.appspot.com/14304043/

[Martin Packman (gz)]

*** Submitted:

ec2: support instance prefix on group filters

The EC2 API documentation allows for alternative spellings of
the group-name and group-id filters on instances. It seems only
the prefixed forms are actually supported when using default VPC
despite claims to the contrary, so add support for them to the
test server infrastructure.

R=rog
CC=
https://codereview.appspot.com/14304043

https://codereview.appspot.com/14304043/

=== modified file 'ec2/ec2t_test.go'
--- ec2/ec2t_test.go	2013-08-15 15:35:52 +0000
+++ ec2/ec2t_test.go	2013-10-02 21:38:26 +0000
@@ -385,12 +385,24 @@
 			},
 			resultIds: ids(0, 1),
 		}, {
+			about: "check that filtering on group id with instance prefix works",
+			filters: []filterSpec{
+				{"instance.group-id", []string{group1.Id}},
+			},
+			resultIds: ids(0, 1),
+		}, {
 			about: "check that filtering on group name works",
 			filters: []filterSpec{
 				{"group-name", []string{group1.Name}},
 			},
 			resultIds: ids(0, 1),
 		}, {
+			about: "check that filtering on group name with instance prefix works",
+			filters: []filterSpec{
+				{"instance.group-name", []string{group1.Name}},
+			},
+			resultIds: ids(0, 1),
+		}, {
 			about: "check that filtering on image id works",
 			filters: []filterSpec{
 				{"image-id", []string{imageId}},

=== modified file 'ec2/ec2test/server.go'
--- ec2/ec2test/server.go	2013-08-15 16:29:50 +0000
+++ ec2/ec2test/server.go	2013-10-02 21:38:26 +0000
@@ -569,14 +569,14 @@
 		return value == "i386", nil
 	case "instance-id":
 		return inst.id() == value, nil
-	case "group-id":
+	case "instance.group-id", "group-id":
 		for _, g := range inst.reservation.groups {
 			if g.id == value {
 				return true, nil
 			}
 		}
 		return false, nil
-	case "group-name":
+	case "instance.group-name", "group-name":
 		for _, g := range inst.reservation.groups {
 			if g.name == value {
 				return true, nil