Merge lp:~fwierzbicki/txaws/break-out-parse-instance into lp:txaws

[Frank Wierzbicki]

This branch breaks out the parsing of an instance from the parsing of an instance set for use in Landscape where we need to extend Instance with our own field.

It also adds a query_factory parameter to FakeEC2Client to match the real EC2Client.

[Duncan McGreggor (oubiwann)]

This is something I wanted to do ages ago, so +1 in general.

One comment for possible change: in the fake client constructor, it might be better to put the new parameter at the end. Esthetically, that's a little odd, but this way it wouldn't interfere with anyone that has been passing unnamed parameters.

That being said, there probably aren't a lot of users of the fake client...

I'll set to approve, pending one more approval.

review: Approve

[lp:~fwierzbicki/txaws/break-out-parse-instance]

82. By Frank Wierzbicki on 2011-04-19

Put query_factory at the end in case someone somewhere is passing parameters by
position. Also fix bug in query_factory assignment.

[Frank Wierzbicki (fwierzbicki)]

> This is something I wanted to do ages ago, so +1 in general.
>
> One comment for possible change: in the fake client constructor, it might be
> better to put the new parameter at the end. Esthetically, that's a little odd,
> but this way it wouldn't interfere with anyone that has been passing unnamed
> parameters.
That's reasonable, I moved query_factory to the end.

> That being said, there probably aren't a lot of users of the fake client...
>
> I'll set to approve, pending one more approval.

[Jamu Kakar (jkakar)]

[1]

+ def _parse_instance(self, instance_data, reservation):

Please add a docstring to this method with @param and @return
components.

Also, it sounds like you'll be calling this method from Landscape
code... do we want it to be private, in that case? It could change
under you if someone else refactors it without knowing its used in
Landscape.

review: Needs Information

[Jamu Kakar (jkakar)]

I guess the perfect fix would be to completely decouple the parsing
logic from the client, but that's probably more work than you want to
do. Anyway, if you're find with calling a private method I'm fine
with this branch, assuming it _parse_instance gets a docstring.

Nice one, +1! :)

review: Approve

[Frank Wierzbicki (fwierzbicki)]

Hey Jamu! We're working together again already :)

> [1]
>
> + def _parse_instance(self, instance_data, reservation):
>
> Please add a docstring to this method with @param and @return
> components.
Oops, right - I added these to all of the methods I touched.

>
> Also, it sounds like you'll be calling this method from Landscape
> code... do we want it to be private, in that case? It could change
> under you if someone else refactors it without knowing its used in
> Landscape.
That sounds reasonable - I made all of the potentially overridden methods for this pass public.

BTW I will probably be changing many of the other methods in the same way in one or more future branches.

[lp:~fwierzbicki/txaws/break-out-parse-instance]

83. By Frank Wierzbicki on 2011-04-19

Add proper docstrings and make methods that we may override public.

[Frank Wierzbicki (fwierzbicki)]

> I guess the perfect fix would be to completely decouple the parsing
> logic from the client, but that's probably more work than you want to
> do. Anyway, if you're find with calling a private method I'm fine
> with this branch, assuming it _parse_instance gets a docstring.
Oops got to work before reading the follow on comment - do you think I should go back to private methods? Going public does mean that the public API for this class is going to probably get to be too big for some...

> Nice one, +1! :)

[lp:~fwierzbicki/txaws/break-out-parse-instance]

84. By Frank Wierzbicki on 2011-04-19

Revert back to private methods for parsing -- later we may want to pull the
parsing out of client.py and then public methods would be reasonable.

[Frank Wierzbicki (fwierzbicki)]

> I guess the perfect fix would be to completely decouple the parsing
> logic from the client, but that's probably more work than you want to
> do. Anyway, if you're find with calling a private method I'm fine
> with this branch, assuming it _parse_instance gets a docstring.
I went ahead and moved those methods back to being private pending the need to move the parsing out.

[Jamu Kakar (jkakar)]

I think keeping them private pending moving the parsing out is best
for now.

=== modified file 'txaws/ec2/client.py'
--- txaws/ec2/client.py	2011-03-21 08:55:48 +0000
+++ txaws/ec2/client.py	2011-04-19 17:35:56 +0000
@@ -43,35 +43,52 @@
         return d.addCallback(self._parse_describe_instances)
 
     def _parse_instances_set(self, root, reservation):
+        """Parse the instances data out of an XML payload.
+        
+           @param root: the root node of the XML payload.
+           @param reservation: the L{Reservation} associtated with the
+                  instances from the response.
+           @return a C{list} of L{Instance}.
+        """
         instances = []
         for instance_data in root.find("instancesSet"):
-            instance_id = instance_data.findtext("instanceId")
-            instance_state = instance_data.find(
-                "instanceState").findtext("name")
-            instance_type = instance_data.findtext("instanceType")
-            image_id = instance_data.findtext("imageId")
-            private_dns_name = instance_data.findtext("privateDnsName")
-            dns_name = instance_data.findtext("dnsName")
-            key_name = instance_data.findtext("keyName")
-            ami_launch_index = instance_data.findtext("amiLaunchIndex")
-            launch_time = instance_data.findtext("launchTime")
-            placement = instance_data.find("placement").findtext(
-                "availabilityZone")
-            products = []
-            product_codes = instance_data.find("productCodes")
-            if product_codes:
-                for product_data in instance_data.find("productCodes"):
-                    products.append(product_data.text)
-            kernel_id = instance_data.findtext("kernelId")
-            ramdisk_id = instance_data.findtext("ramdiskId")
-            instance = model.Instance(
-                instance_id, instance_state, instance_type, image_id,
-                private_dns_name, dns_name, key_name, ami_launch_index,
-                launch_time, placement, products, kernel_id, ramdisk_id,
-                reservation=reservation)
-            instances.append(instance)
+            instances.append(self._parse_instance(instance_data, reservation))
         return instances
 
+    def _parse_instance(self, instance_data, reservation):
+        """Parse the instance data out of an XML payload.
+        
+           @param instance_data: an XML node containing instance data.
+           @param reservation: the L{Reservation} associatated with the
+                  instance.
+           @return an L{Instance}.
+        """
+        instance_id = instance_data.findtext("instanceId")
+        instance_state = instance_data.find(
+            "instanceState").findtext("name")
+        instance_type = instance_data.findtext("instanceType")
+        image_id = instance_data.findtext("imageId")
+        private_dns_name = instance_data.findtext("privateDnsName")
+        dns_name = instance_data.findtext("dnsName")
+        key_name = instance_data.findtext("keyName")
+        ami_launch_index = instance_data.findtext("amiLaunchIndex")
+        launch_time = instance_data.findtext("launchTime")
+        placement = instance_data.find("placement").findtext(
+            "availabilityZone")
+        products = []
+        product_codes = instance_data.find("productCodes")
+        if product_codes:
+            for product_data in instance_data.find("productCodes"):
+                products.append(product_data.text)
+        kernel_id = instance_data.findtext("kernelId")
+        ramdisk_id = instance_data.findtext("ramdiskId")
+        instance = model.Instance(
+            instance_id, instance_state, instance_type, image_id,
+            private_dns_name, dns_name, key_name, ami_launch_index,
+            launch_time, placement, products, kernel_id, ramdisk_id,
+            reservation=reservation)
+        return instance
+
     def _parse_describe_instances(self, xml_bytes):
         """
         Parse the reservations XML payload that is returned from an AWS

=== modified file 'txaws/testing/ec2.py'
--- txaws/testing/ec2.py	2009-10-14 14:41:28 +0000
+++ txaws/testing/ec2.py	2011-04-19 17:35:56 +0000
@@ -10,13 +10,18 @@
 
 from txaws.ec2.model import Keypair, SecurityGroup
 
+
 class FakeEC2Client(object):
 
     def __init__(self, creds, endpoint, instances=None, keypairs=None,
                  volumes=None, key_material="", security_groups=None,
-                 snapshots=None, addresses=None, availability_zones=None):
+                 snapshots=None, addresses=None, availability_zones=None,
+                 query_factory=None):
+
         self.creds = creds
         self.endpoint = endpoint
+        self.query_factory = query_factory
+
         self.instances = instances or []
         self.keypairs = keypairs or []
         self.keypairs_deleted = []
@@ -141,4 +146,3 @@
         except:
             failure = Failure()
         return fail(failure)
-