txAWS

Merge lp://staging/~franciscosouza/txaws/txaws-secgroupid into lp://staging/txaws

txaws-secgroupid
Merge into trunk

Proposed by Francisco Souza on 2012-11-08

Status:	Needs review
Proposed branch:	lp://staging/~franciscosouza/txaws/txaws-secgroupid
Merge into:	lp://staging/txaws
Diff against target:	279 lines (+104/-14) 5 files modified txaws/ec2/client.py (+20/-6) txaws/ec2/model.py (+2/-1) txaws/ec2/tests/test_client.py (+71/-3) txaws/ec2/tests/test_model.py (+6/-4) txaws/testing/payload.py (+5/-0)
To merge this branch:	bzr merge lp://staging/~franciscosouza/txaws/txaws-secgroupid
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
txAWS Committers		2012-11-08	Pending
Review via email: mp+133542@code.staging.launchpad.net

Description of the change

ec2: recognize ID on security groups

When using VPC, security groups are identified by ID, not name.

https://codereview.appspot.com/6822097/

Revision history for this message

Chico (franciscossouza) wrote on 2012-11-08:

Download full text (9.5 KiB)

Reviewers: mp+133542_code.launchpad.net,

Message:
Please take a look.

Description:
ec2: recognize ID on security groups

When using VPC, security groups are identified by ID, not name.

https://code.launchpad.net/~franciscosouza/txaws/txaws-secgroupid/+merge/133542

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M txaws/ec2/client.py
   M txaws/ec2/model.py
   M txaws/ec2/tests/test_client.py
   M txaws/ec2/tests/test_model.py
   M txaws/testing/payload.py

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: txaws/ec2/client.py
=== modified file 'txaws/ec2/client.py'
--- txaws/ec2/client.py 2012-05-05 00:17:02 +0000
+++ txaws/ec2/client.py 2012-11-08 18:46:21 +0000
@@ -125,13 +125,18 @@
d = query.submit()
return d.addCallback(self.parser.truth_return)

- def delete_security_group(self, name):
+ def delete_security_group(self, name=None, id=None):
          """
          @param name: Name of the new security group.
          @return: A C{Deferred} that will fire with a truth value for the
              success of the operation.
          """
- parameter = {"GroupName": name}
+ if name:
+ parameter = {"GroupName": name}
+ elif id:
+ parameter = {"GroupId": id}
+ else:
+ raise ValueError("You must provide either the security group
name or id")
          query = self.query_factory(
              action="DeleteSecurityGroup", creds=self.creds,
              endpoint=self.endpoint, other_params=parameter)
@@ -670,6 +675,7 @@
          root = XML(xml_bytes)
          result = []
          for group_info in root.findall("securityGroupInfo/item"):
+ id = group_info.findtext("groupId")
              name = group_info.findtext("groupName")
              description = group_info.findtext("groupDescription")
              owner_id = group_info.findtext("ownerId")
@@ -709,7 +715,7 @@
                                for user_id, group_name in allowed_groups]

              security_group = model.SecurityGroup(
- name, description, owner_id=owner_id,
+ id, name, description, owner_id=owner_id,
                  groups=allowed_groups, ips=allowed_ips)
              result.append(security_group)
          return result

Index: txaws/ec2/model.py
=== modified file 'txaws/ec2/model.py'
--- txaws/ec2/model.py 2012-03-02 22:00:10 +0000
+++ txaws/ec2/model.py 2012-11-08 18:37:45 +0000
@@ -80,7 +80,8 @@
      @ivar allowed_ips: The sequence of L{IPPermission} instances for this
          security group.
      """
- def __init__(self, name, description, owner_id="", groups=None,
ips=None):
+ def __init__(self, id, name, description, owner_id="", groups=None,
ips=None):
+ self.id = id
          self.name = name
          self.description = description
     ...

Reviewers: mp+133542_code.launchpad.net,

Message:
Please take a look.

Description:
ec2: recognize ID on security groups

When using VPC, security groups are identified by ID, not name.

https://code.launchpad.net/~franciscosouza/txaws/txaws-secgroupid/+merge/133542

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M txaws/ec2/client.py
   M txaws/ec2/model.py
   M txaws/ec2/tests/test_client.py
   M txaws/ec2/tests/test_model.py
   M txaws/testing/payload.py

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: andreas@canonical.com-20121030114050-j8a7328o10mq91x3
+New revision: f@souza.cc-20121108184621-ik5cwp3q1zbf184v

Index: txaws/ec2/client.py
=== modified file 'txaws/ec2/client.py'
--- txaws/ec2/client.py	2012-05-05 00:17:02 +0000
+++ txaws/ec2/client.py	2012-11-08 18:46:21 +0000
@@ -125,13 +125,18 @@
          d = query.submit()
          return d.addCallback(self.parser.truth_return)

-    def delete_security_group(self, name):
+    def delete_security_group(self, name=None, id=None):
          """
          @param name: Name of the new security group.
          @return: A C{Deferred} that will fire with a truth value for the
              success of the operation.
          """
-        parameter = {"GroupName":  name}
+        if name:
+            parameter = {"GroupName":  name}
+        elif id:
+            parameter = {"GroupId": id}
+        else:
+            raise ValueError("You must provide either the security group  
name or id")
          query = self.query_factory(
              action="DeleteSecurityGroup", creds=self.creds,
              endpoint=self.endpoint, other_params=parameter)
@@ -670,6 +675,7 @@
          root = XML(xml_bytes)
          result = []
          for group_info in root.findall("securityGroupInfo/item"):
+            id = group_info.findtext("groupId")
              name = group_info.findtext("groupName")
              description = group_info.findtext("groupDescription")
              owner_id = group_info.findtext("ownerId")
@@ -709,7 +715,7 @@
                                for user_id, group_name in allowed_groups]

security_group = model.SecurityGroup(
-                name, description, owner_id=owner_id,
+                id, name, description, owner_id=owner_id,
                  groups=allowed_groups, ips=allowed_ips)
              result.append(security_group)
          return result

Index: txaws/ec2/model.py
=== modified file 'txaws/ec2/model.py'
--- txaws/ec2/model.py	2012-03-02 22:00:10 +0000
+++ txaws/ec2/model.py	2012-11-08 18:37:45 +0000
@@ -80,7 +80,8 @@
      @ivar allowed_ips: The sequence of L{IPPermission} instances for this
          security group.
      """
-    def __init__(self, name, description, owner_id="", groups=None,  
ips=None):
+    def __init__(self, id, name, description, owner_id="", groups=None,  
ips=None):
+        self.id = id
          self.name = name
          self.description = description
          self.owner_id = owner_id

Index: txaws/testing/payload.py
=== modified file 'txaws/testing/payload.py'
--- txaws/testing/payload.py	2012-05-16 02:47:12 +0000
+++ txaws/testing/payload.py	2012-11-08 18:37:45 +0000
@@ -213,6 +213,7 @@
           <fromPort/>
          </item>
        </ipPermissions>
+      <groupId>sg-a1a1a1</groupId>
        <groupName>WebServers</groupName>
        <groupDescription>Web servers</groupDescription>
        <ownerId>UYY3TLBUXIEON5NQVUUX6OMPWBZIQNFM</ownerId>
@@ -228,6 +229,7 @@
    <securityGroupInfo>
      <item>
        <ownerId>UYY3TLBUXIEON5NQVUUX6OMPWBZIQNFM</ownerId>
+      <groupId>sg-a1a1a1</groupId>
        <groupName>WebServers</groupName>
        <groupDescription>Web Servers</groupDescription>
        <ipPermissions>
@@ -256,6 +258,7 @@
    <securityGroupInfo>
      <item>
        <ownerId>UYY3TLBUXIEON5NQVUUX6OMPWBZIQNFM</ownerId>
+      <groupId>sg-a1a1a1</groupId>
        <groupName>MessageServers</groupName>
        <groupDescription>Message Servers</groupDescription>
        <ipPermissions>
@@ -274,6 +277,7 @@
      </item>
      <item>
        <ownerId>UYY3TLBUXIEON5NQVUUX6OMPWBZIQNFM</ownerId>
+      <groupId>sg-c3c3c3</groupId>
        <groupName>WebServers</groupName>
        <groupDescription>Web Servers</groupDescription>
        <ipPermissions>

Index: txaws/ec2/tests/test_client.py
=== modified file 'txaws/ec2/tests/test_client.py'
--- txaws/ec2/tests/test_client.py	2012-03-02 22:00:10 +0000
+++ txaws/ec2/tests/test_client.py	2012-11-08 18:46:21 +0000
@@ -400,6 +400,7 @@

def check_results(security_groups):
              [security_group] = security_groups
+            self.assertEquals(security_group.id, "sg-a1a1a1")
              self.assertEquals(security_group.owner_id,
                                "UYY3TLBUXIEON5NQVUUX6OMPWBZIQNFM")
              self.assertEquals(security_group.name, "WebServers")
@@ -440,6 +441,7 @@
              security_group = security_groups[0]
              self.assertEquals(security_group.owner_id,
                                "UYY3TLBUXIEON5NQVUUX6OMPWBZIQNFM")
+            self.assertEquals(security_group.id, "sg-a1a1a1")
              self.assertEquals(security_group.name, "MessageServers")
              self.assertEquals(security_group.description, "Message  
Servers")
              self.assertEquals(security_group.allowed_groups, [])
@@ -451,6 +453,7 @@
              security_group = security_groups[1]
              self.assertEquals(security_group.owner_id,
                                "UYY3TLBUXIEON5NQVUUX6OMPWBZIQNFM")
+            self.assertEquals(security_group.id, "sg-c3c3c3")
              self.assertEquals(security_group.name, "WebServers")
              self.assertEquals(security_group.description, "Web Servers")
              self.assertEquals([(pair.user_id, pair.group_name)
@@ -590,7 +593,7 @@
              "The group for the web server farm.")
          return self.assertTrue(d)

-    def test_delete_security_group(self):
+    def test_delete_security_group_using_name(self):
          """
          L{EC2Client.delete_security_group} returns a C{Deferred} that
          eventually fires with a true value, indicating the success of the
@@ -615,6 +618,40 @@
          d = ec2.delete_security_group("WebServers")
          return self.assertTrue(d)

+    def test_delete_security_group_using_id(self):
+        """
+        L{EC2Client.delete_security_group} returns a C{Deferred} that
+        eventually fires with a true value, indicating the success of the
+        operation.
+        """
+        class StubQuery(object):
+
+            def __init__(stub, action="", creds=None, endpoint=None,
+                         other_params={}):
+                self.assertEqual(action, "DeleteSecurityGroup")
+                self.assertEqual(creds.access_key, "foo")
+                self.assertEqual(creds.secret_key, "bar")
+                self.assertEqual(other_params, {
+                    "GroupId": "sg-a1a1a1",
+                    })
+
+            def submit(self):
+                return succeed(payload.sample_delete_security_group)
+
+        creds = AWSCredentials("foo", "bar")
+        ec2 = client.EC2Client(creds, query_factory=StubQuery)
+        d = ec2.delete_security_group(id="sg-a1a1a1")
+        return self.assertTrue(d)
+
+    def test_delete_security_group_without_id_and_name(self):
+        creds = AWSCredentials("foo", "bar")
+        ec2 = client.EC2Client(creds)
+        error = self.assertRaises(ValueError, ec2.delete_security_group)
+        self.assertEquals(
+            str(error),
+            "You must provide either the security group name or id",
+        )
+
      def test_delete_security_group_failure(self):
          """
          L{EC2Client.delete_security_group} returns a C{Deferred} that

Index: txaws/ec2/tests/test_model.py
=== modified file 'txaws/ec2/tests/test_model.py'
--- txaws/ec2/tests/test_model.py	2012-01-23 01:04:25 +0000
+++ txaws/ec2/tests/test_model.py	2012-11-08 18:37:45 +0000
@@ -8,7 +8,8 @@
  class SecurityGroupTestCase(TXAWSTestCase):

def test_creation_defaults(self):
-        group = model.SecurityGroup("name", "desc")
+        group = model.SecurityGroup("sg-a3f2", "name", "desc")
+        self.assertEquals(group.id, "sg-a3f2")
          self.assertEquals(group.name, "name")
          self.assertEquals(group.description, "desc")
          self.assertEquals(group.owner_id, "")
@@ -18,14 +19,15 @@
      def test_creation_all_parameters(self):
          user = "somegal24"
          other_groups = [
-            model.SecurityGroup("other1", "another group 1"),
-            model.SecurityGroup("other2", "another group 2")]
+            model.SecurityGroup("sg-other1", "other1", "another group 1"),
+            model.SecurityGroup("sg-other2", "other2", "another group 2")]
          user_group_pairs = [
              model.UserIDGroupPair(user, other_groups[0].name),
              model.UserIDGroupPair(user, other_groups[1].name)]
          ips = [model.IPPermission("tcp", "80", "80", "10.0.1.0/24")]
          group = model.SecurityGroup(
-            "name", "desc", owner_id="me", groups=user_group_pairs,  
ips=ips)
+            "id", "name", "desc", owner_id="me", groups=user_group_pairs,  
ips=ips)
+        self.assertEquals(group.id, "id")
          self.assertEquals(group.name, "name")
          self.assertEquals(group.description, "desc")
          self.assertEquals(group.owner_id, "me")

Revision history for this message

Chico (franciscossouza) wrote on 2012-11-08:

On 2012/11/08 18:50:48, fss wrote:
> Please take a look.

Please hold on, I forgot to include create_security_group change.

https://codereview.appspot.com/6822097/