Merge lp:~oubiwann/txaws/415486-reservation-object into lp:~txawsteam/txaws/trunk
- 415486-reservation-object
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Merge reported by: | Duncan McGreggor | ||||
Merged at revision: | not available | ||||
Proposed branch: | lp:~oubiwann/txaws/415486-reservation-object | ||||
Merge into: | lp:~txawsteam/txaws/trunk | ||||
Diff against target: | None lines | ||||
To merge this branch: | bzr merge lp:~oubiwann/txaws/415486-reservation-object | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jamu Kakar (community) | Approve | ||
Thomas Herve (community) | Approve | ||
Review via email: mp+10338@code.launchpad.net |
Commit message
Description of the change
Duncan McGreggor (oubiwann) wrote : | # |
- 9. By Duncan McGreggor
-
Updated the gtk client to wrap the new API change.
Robert Collins (lifeless) wrote : | # |
As far as I could tell only one reservation is returned, not N?
If I'm wrong this is fine, if I'm correct, then it would be nicer to
return the reservation rather than an iterable of reservations.
-Rob
Jamu Kakar (jkakar) wrote : | # |
[1]
+ def __init__(self, reservation_id, owner_id, groups=[], instances=[]):
The default empty list values for groups and instances will be
inadvertently shared by all Reservation instances that don't provide
explicit values during instantiation. I recommend you use None and
set the value in the body of the constructor as necessary.
[2]
+ def test_parse_
+ ec2 = client.
+ results = ec2._parse_
+ self.check_
It would be nice to avoid accessing the private _parse_reservation
method by passing a query_factory to EC2Client that returns a
FakeQuery. FakeQuery.submit could return a succeeded deferred and
you could call ec2.describe_
result you want to make assertions about.
+1 considering #1.
Thomas Herve (therve) wrote : | # |
> As far as I could tell only one reservation is returned, not N?
No, you get one reservation per call to CreateInstances. If you're calling CreateInstances several times, you're getting several reservations.
The reservations are what is holding the information about the security groups used, for example.
- 10. By Duncan McGreggor
-
Merged from trunk and resolved conflicts.
Robert Collins (lifeless) wrote : | # |
On Tue, 2009-08-18 at 20:34 +0000, Thomas Herve wrote:
> > As far as I could tell only one reservation is returned, not N?
>
> No, you get one reservation per call to CreateInstances. If you're calling CreateInstances several times, you're getting several reservations.
> The reservations are what is holding the information about the security groups used, for example.
Thats 'yes' then - a call to CreateInstances returns one reservation.
Having our thunk to create instances return a _list of reservations_
makes the API harder to work with.
-Rob
- 11. By Duncan McGreggor
-
Changed default values in signature so that groups and instances wouldn't be
shared across all Reservation objects.
Thomas Herve (therve) wrote : | # |
> On Tue, 2009-08-18 at 20:34 +0000, Thomas Herve wrote:
> > > As far as I could tell only one reservation is returned, not N?
> >
> > No, you get one reservation per call to CreateInstances. If you're calling
> CreateInstances several times, you're getting several reservations.
> > The reservations are what is holding the information about the security
> groups used, for example.
>
> Thats 'yes' then - a call to CreateInstances returns one reservation.
>
> Having our thunk to create instances return a _list of reservations_
> makes the API harder to work with.
We're not *creating* instances, we're describing them. A call to CreateInstances may return a list of instances, but it's not what we're doing right now.
- 12. By Duncan McGreggor
-
Finished the fix from the last commit (jkakar 1).
Duncan McGreggor (oubiwann) wrote : | # |
> [1]
>
> + def __init__(self, reservation_id, owner_id, groups=[], instances=[]):
>
> The default empty list values for groups and instances will be
> inadvertently shared by all Reservation instances that don't provide
> explicit values during instantiation. I recommend you use None and
> set the value in the body of the constructor as necessary.
Nice catch! Fixed.
> [2]
>
> + def test_parse_
> + ec2 = client.
> + results = ec2._parse_
> + self.check_
>
> It would be nice to avoid accessing the private _parse_reservation
> method by passing a query_factory to EC2Client that returns a
> FakeQuery. FakeQuery.submit could return a succeeded deferred and
> you could call ec2.describe_
> result you want to make assertions about.
This is interesting. I'm not completely sold on it yet... but I'll create a ticket for this.
Robert Collins (lifeless) wrote : | # |
We just chatted on IRC.
It would be nice to keep returning a list of instances. We can expose
the reservation as a reservation attribute on each instance.
-Rob
Duncan McGreggor (oubiwann) wrote : | # |
> > On Tue, 2009-08-18 at 20:34 +0000, Thomas Herve wrote:
> > > > As far as I could tell only one reservation is returned, not N?
> > >
> > > No, you get one reservation per call to CreateInstances. If you're calling
> > CreateInstances several times, you're getting several reservations.
> > > The reservations are what is holding the information about the security
> > groups used, for example.
> >
> > Thats 'yes' then - a call to CreateInstances returns one reservation.
> >
> > Having our thunk to create instances return a _list of reservations_
> > makes the API harder to work with.
>
> We're not *creating* instances, we're describing them. A call to
> CreateInstances may return a list of instances, but it's not what we're doing
> right now.
There was some confusion about this which got sorted out on IRC. Robert then proposed an excellent idea that we all liked. Here's the IRC chat:
[18:14] <therve`> lifeless: the API we're calling is DescribeInstances
[18:14] <therve`> this one returns a list of reservations
[18:14] <lifeless> ok
[18:14] <lifeless> its still a little ugly to work with but I can see the rationale
[18:14] <therve`> yeah, the problem is that the reservation actually holds important information
[18:15] <therve`> which we can't get anywhere else
[18:15] <lifeless> we could fold that down to the instance
[18:15] <lifeless> if the info is only a field or two
[18:15] <obiwann> lifeless: yeah, that was one of my thoughts too... still not sure how I feel about it
[18:15] <lifeless> that is
[18:15] <lifeless> rather than reservations[
[18:15] <lifeless> have instances[
[18:16] <therve`> this is a valid idea
[18:16] <obiwann> yeah, I kinda like that
[18:16] <obiwann> instances[
- 13. By Duncan McGreggor
-
Made the reservation for a given instance an attribute on that instance.
- 14. By Duncan McGreggor
-
Renamed the functions associated with parsing the AWS reponse, due to the
change in functionality. - 15. By Duncan McGreggor
-
Rolled back the now unnecessary change to the gtk code.
Duncan McGreggor (oubiwann) wrote : | # |
> We just chatted on IRC.
>
> It would be nice to keep returning a list of instances. We can expose
> the reservation as a reservation attribute on each instance.
>
> -Rob
Hey Rob, when you get a chance, I've updated the code (and pushed it) according to my understanding of your suggestions. Let me know if it's what you had in mind.
Thanks!
Robert Collins (lifeless) wrote : | # |
Looks good to me - thanks.
-Rob
Preview Diff
1 | === modified file 'txaws/ec2/client.py' | |||
2 | --- txaws/ec2/client.py 2009-04-28 11:52:57 +0000 | |||
3 | +++ txaws/ec2/client.py 2009-08-18 19:08:31 +0000 | |||
4 | @@ -14,22 +14,37 @@ | |||
5 | 14 | from txaws.util import iso8601time, XML | 14 | from txaws.util import iso8601time, XML |
6 | 15 | 15 | ||
7 | 16 | 16 | ||
8 | 17 | class Reservation(object): | ||
9 | 18 | """An Amazon EC2 Reservation. | ||
10 | 19 | |||
11 | 20 | @attrib reservation_id: Unique ID of the reservation. | ||
12 | 21 | @attrib owner_id: AWS Access Key ID of the user who owns the reservation. | ||
13 | 22 | @attrib groups: A list of security groups. | ||
14 | 23 | @attrib instances: A list of C{Instance}s. | ||
15 | 24 | """ | ||
16 | 25 | def __init__(self, reservation_id, owner_id, groups=[], instances=[]): | ||
17 | 26 | self.reservation_id = reservation_id | ||
18 | 27 | self.owner_id = owner_id | ||
19 | 28 | self.groups = groups | ||
20 | 29 | self.instances = instances | ||
21 | 30 | |||
22 | 31 | |||
23 | 17 | class Instance(object): | 32 | class Instance(object): |
24 | 18 | """An Amazon EC2 Instance. | 33 | """An Amazon EC2 Instance. |
25 | 19 | 34 | ||
28 | 20 | :attrib instanceId: The instance ID of this instance. | 35 | @attrib instance_id: The instance ID of this instance. |
29 | 21 | :attrib instanceState: The state of this instance. | 36 | @attrib instance_state: The state of this instance. |
30 | 22 | """ | 37 | """ |
31 | 23 | 38 | ||
35 | 24 | def __init__(self, instanceId, instanceState): | 39 | def __init__(self, instance_id, instance_state): |
36 | 25 | self.instanceId = instanceId | 40 | self.instance_id = instance_id |
37 | 26 | self.instanceState = instanceState | 41 | self.instance_state = instance_state |
38 | 27 | 42 | ||
39 | 28 | 43 | ||
40 | 29 | class EC2Client(object): | 44 | class EC2Client(object): |
41 | 30 | """A client for EC2.""" | 45 | """A client for EC2.""" |
42 | 31 | 46 | ||
44 | 32 | NS = '{http://ec2.amazonaws.com/doc/2008-12-01/}' | 47 | name_space = '{http://ec2.amazonaws.com/doc/2008-12-01/}' |
45 | 33 | 48 | ||
46 | 34 | def __init__(self, creds=None, query_factory=None): | 49 | def __init__(self, creds=None, query_factory=None): |
47 | 35 | """Create an EC2Client. | 50 | """Create an EC2Client. |
48 | @@ -50,19 +65,38 @@ | |||
49 | 50 | """Describe current instances.""" | 65 | """Describe current instances.""" |
50 | 51 | q = self.query_factory('DescribeInstances', self.creds) | 66 | q = self.query_factory('DescribeInstances', self.creds) |
51 | 52 | d = q.submit() | 67 | d = q.submit() |
53 | 53 | return d.addCallback(self._parse_Reservation) | 68 | return d.addCallback(self._parse_reservation) |
54 | 54 | 69 | ||
56 | 55 | def _parse_Reservation(self, xml_bytes): | 70 | def _parse_reservation(self, xml_bytes): |
57 | 56 | root = XML(xml_bytes) | 71 | root = XML(xml_bytes) |
59 | 57 | result = [] | 72 | results = [] |
60 | 58 | # May be a more elegant way to do this: | 73 | # May be a more elegant way to do this: |
68 | 59 | for reservation in root.find(self.NS + 'reservationSet'): | 74 | for reservation_data in root.find(self.name_space + 'reservationSet'): |
69 | 60 | for instance in reservation.find(self.NS + 'instancesSet'): | 75 | # Get the security group information. |
70 | 61 | instanceId = instance.findtext(self.NS + 'instanceId') | 76 | groups = [] |
71 | 62 | instanceState = instance.find( | 77 | for group_data in reservation_data.find( |
72 | 63 | self.NS + 'instanceState').findtext(self.NS + 'name') | 78 | self.name_space + 'groupSet'): |
73 | 64 | result.append(Instance(instanceId, instanceState)) | 79 | group_id = group_data.findtext(self.name_space + 'groupId') |
74 | 65 | return result | 80 | groups.append(group_id) |
75 | 81 | # Get the list of instances. | ||
76 | 82 | instances = [] | ||
77 | 83 | for instance_data in reservation_data.find( | ||
78 | 84 | self.name_space + 'instancesSet'): | ||
79 | 85 | instance_id = instance_data.findtext( | ||
80 | 86 | self.name_space + 'instanceId') | ||
81 | 87 | instance_state = instance_data.find( | ||
82 | 88 | self.name_space + 'instanceState').findtext( | ||
83 | 89 | self.name_space + 'name') | ||
84 | 90 | instances.append(Instance(instance_id, instance_state)) | ||
85 | 91 | # Create a reservation object with the parsed data. | ||
86 | 92 | reservation = Reservation( | ||
87 | 93 | reservation_id=reservation_data.findtext( | ||
88 | 94 | self.name_space + 'reservationId'), | ||
89 | 95 | owner_id=reservation_data.findtext( | ||
90 | 96 | self.name_space + 'ownerId'), | ||
91 | 97 | groups=groups, instances=instances) | ||
92 | 98 | results.append(reservation) | ||
93 | 99 | return results | ||
94 | 66 | 100 | ||
95 | 67 | def terminate_instances(self, *instance_ids): | 101 | def terminate_instances(self, *instance_ids): |
96 | 68 | """Terminate some instances. | 102 | """Terminate some instances. |
97 | @@ -82,12 +116,14 @@ | |||
98 | 82 | root = XML(xml_bytes) | 116 | root = XML(xml_bytes) |
99 | 83 | result = [] | 117 | result = [] |
100 | 84 | # May be a more elegant way to do this: | 118 | # May be a more elegant way to do this: |
103 | 85 | for instance in root.find(self.NS + 'instancesSet'): | 119 | for instance in root.find(self.name_space + 'instancesSet'): |
104 | 86 | instanceId = instance.findtext(self.NS + 'instanceId') | 120 | instanceId = instance.findtext(self.name_space + 'instanceId') |
105 | 87 | previousState = instance.find( | 121 | previousState = instance.find( |
107 | 88 | self.NS + 'previousState').findtext(self.NS + 'name') | 122 | self.name_space + 'previousState').findtext( |
108 | 123 | self.name_space + 'name') | ||
109 | 89 | shutdownState = instance.find( | 124 | shutdownState = instance.find( |
111 | 90 | self.NS + 'shutdownState').findtext(self.NS + 'name') | 125 | self.name_space + 'shutdownState').findtext( |
112 | 126 | self.name_space + 'name') | ||
113 | 91 | result.append((instanceId, previousState, shutdownState)) | 127 | result.append((instanceId, previousState, shutdownState)) |
114 | 92 | return result | 128 | return result |
115 | 93 | 129 | ||
116 | 94 | 130 | ||
117 | === modified file 'txaws/ec2/tests/test_client.py' | |||
118 | --- txaws/ec2/tests/test_client.py 2009-04-28 11:52:57 +0000 | |||
119 | +++ txaws/ec2/tests/test_client.py 2009-08-18 19:08:31 +0000 | |||
120 | @@ -9,6 +9,7 @@ | |||
121 | 9 | from txaws.ec2 import client | 9 | from txaws.ec2 import client |
122 | 10 | from txaws.tests import TXAWSTestCase | 10 | from txaws.tests import TXAWSTestCase |
123 | 11 | 11 | ||
124 | 12 | |||
125 | 12 | sample_describe_instances_result = """<?xml version="1.0"?> | 13 | sample_describe_instances_result = """<?xml version="1.0"?> |
126 | 13 | <DescribeInstancesResponse xmlns="http://ec2.amazonaws.com/doc/2008-12-01/"> | 14 | <DescribeInstancesResponse xmlns="http://ec2.amazonaws.com/doc/2008-12-01/"> |
127 | 14 | <requestId>52b4c730-f29f-498d-94c1-91efb75994cc</requestId> | 15 | <requestId>52b4c730-f29f-498d-94c1-91efb75994cc</requestId> |
128 | @@ -49,6 +50,7 @@ | |||
129 | 49 | </DescribeInstancesResponse> | 50 | </DescribeInstancesResponse> |
130 | 50 | """ | 51 | """ |
131 | 51 | 52 | ||
132 | 53 | |||
133 | 52 | sample_terminate_instances_result = """<?xml version="1.0"?> | 54 | sample_terminate_instances_result = """<?xml version="1.0"?> |
134 | 53 | <TerminateInstancesResponse xmlns="http://ec2.amazonaws.com/doc/2008-12-01/"> | 55 | <TerminateInstancesResponse xmlns="http://ec2.amazonaws.com/doc/2008-12-01/"> |
135 | 54 | <instancesSet> | 56 | <instancesSet> |
136 | @@ -79,6 +81,20 @@ | |||
137 | 79 | """ | 81 | """ |
138 | 80 | 82 | ||
139 | 81 | 83 | ||
140 | 84 | class ReservationTestCase(TXAWSTestCase): | ||
141 | 85 | |||
142 | 86 | def test_reservation_creation(self): | ||
143 | 87 | reservation = client.Reservation( | ||
144 | 88 | "id1", "owner", groups=["one", "two"], | ||
145 | 89 | instances=[client.Instance("id2", "state")]) | ||
146 | 90 | self.assertEquals(reservation.reservation_id, "id1") | ||
147 | 91 | self.assertEquals(reservation.owner_id, "owner") | ||
148 | 92 | self.assertEquals(reservation.groups, ["one", "two"]) | ||
149 | 93 | instance = reservation.instances[0] | ||
150 | 94 | self.assertEquals(instance.instance_id, "id2") | ||
151 | 95 | self.assertEquals(instance.instance_state, "state") | ||
152 | 96 | |||
153 | 97 | |||
154 | 82 | class TestEC2Client(TXAWSTestCase): | 98 | class TestEC2Client(TXAWSTestCase): |
155 | 83 | 99 | ||
156 | 84 | def test_init_no_creds(self): | 100 | def test_init_no_creds(self): |
157 | @@ -95,6 +111,21 @@ | |||
158 | 95 | ec2 = client.EC2Client(creds=creds) | 111 | ec2 = client.EC2Client(creds=creds) |
159 | 96 | self.assertEqual(creds, ec2.creds) | 112 | self.assertEqual(creds, ec2.creds) |
160 | 97 | 113 | ||
161 | 114 | def check_parsed_reservations(self, results): | ||
162 | 115 | reservation = results[0] | ||
163 | 116 | self.assertEquals(reservation.reservation_id, "r-cf24b1a6") | ||
164 | 117 | self.assertEquals(reservation.owner_id, "123456789012") | ||
165 | 118 | instance = reservation.instances[0] | ||
166 | 119 | self.assertEquals(instance.instance_id, "i-abcdef01") | ||
167 | 120 | self.assertEquals(instance.instance_state, "running") | ||
168 | 121 | group = reservation.groups[0] | ||
169 | 122 | self.assertEquals(group, "default") | ||
170 | 123 | |||
171 | 124 | def test_parse_reservation(self): | ||
172 | 125 | ec2 = client.EC2Client(creds='foo') | ||
173 | 126 | results = ec2._parse_reservation(sample_describe_instances_result) | ||
174 | 127 | self.check_parsed_reservations(results) | ||
175 | 128 | |||
176 | 98 | def test_describe_instances(self): | 129 | def test_describe_instances(self): |
177 | 99 | class StubQuery(object): | 130 | class StubQuery(object): |
178 | 100 | def __init__(stub, action, creds): | 131 | def __init__(stub, action, creds): |
179 | @@ -104,11 +135,7 @@ | |||
180 | 104 | return succeed(sample_describe_instances_result) | 135 | return succeed(sample_describe_instances_result) |
181 | 105 | ec2 = client.EC2Client(creds='foo', query_factory=StubQuery) | 136 | ec2 = client.EC2Client(creds='foo', query_factory=StubQuery) |
182 | 106 | d = ec2.describe_instances() | 137 | d = ec2.describe_instances() |
188 | 107 | def check_instances(reservation): | 138 | d.addCallback(self.check_parsed_reservations) |
184 | 108 | self.assertEqual(1, len(reservation)) | ||
185 | 109 | self.assertEqual('i-abcdef01', reservation[0].instanceId) | ||
186 | 110 | self.assertEqual('running', reservation[0].instanceState) | ||
187 | 111 | d.addCallback(check_instances) | ||
189 | 112 | return d | 139 | return d |
190 | 113 | 140 | ||
191 | 114 | def test_terminate_instances(self): | 141 | def test_terminate_instances(self): |
This is ready for review.