Status: | Merged |
---|---|
Approved by: | Raphaël Badin |
Approved revision: | 64 |
Merged at revision: | 69 |
Proposed branch: | lp:~rvb/gwacl/poller |
Merge into: | lp:gwacl |
Diff against target: |
705 lines (+394/-63) 8 files modified
managementapi.go (+2/-2) managementapi_test.go (+19/-15) poller.go (+92/-0) poller_test.go (+187/-0) test_helpers.go (+64/-0) x509session.go (+7/-7) x509session_test.go (+9/-39) xmlobjects.go (+14/-0) |
To merge this branch: | bzr merge lp:~rvb/gwacl/poller |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jeroen T. Vermeulen (community) | Approve | ||
Review via email: mp+155680@code.launchpad.net |
Commit message
Add generic poller utility. Add a specialized poller to poll the Windows Azure server until an operation is completed.
Description of the change
The main change in this branch is the addition of the file poller.go (and the related tests) which adds a generic Poller interface and a generic method to actually perform the polling plus an specialized implementation for polling the Windows Azure server until a specific operation is completed.
A second branch will:
- change the asynchronous operations from managementapi.go so that they will return the corresponding operation id.
- update live_example_
This change was pre-imp'ed with Jeroen but instead of returning channels like we originally thought StartPolling would do, I decided to create a blocking method. It's then up to the caller to use goroutines if he does not want to block during the polling. This is — I think — a better design because it leaves it up to the caller to create and manage channels as he likes.
I've moved the test utilities rig*** in the test_helpers.go file because they are now used in many places.
Also, I changed the session's get/post/put so that they now return the full response instead of just the body. We need that in order to check the return status (and we will also need it to extract the operationID value from the headers).
Jeroen T. Vermeulen (jtv) wrote : | # |
Jeroen T. Vermeulen (jtv) wrote : | # |
I think I was wrong on one point, actually: "wants to not block" would be the correct geek-speak. :-)
Jeroen T. Vermeulen (jtv) wrote : | # |
Oh, and ESPECIALLY don't arbitrarily switch genders for the same person¹. Did somebody make you a bet that it would set me off? :)
—
¹) With the appropriate exception for trans-gender individuals, of course.
Raphaël Badin (rvb) wrote : | # |
Wording is fixed… can I haz code review? ;)
Raphaël Badin (rvb) wrote : | # |
> Oh, and ESPECIALLY don't arbitrarily switch genders for the same person¹. Did
> somebody make you a bet that it would set me off? :)
Well, I guess I know which button to push if I want to set you off now :).
Jeroen T. Vermeulen (jtv) wrote : | # |
Oh, no worries. Just about any button will do for that.
Jeroen T. Vermeulen (jtv) wrote : | # |
You mentioned that you were taking this channel-less approach and I think it was the right choice.
Overall a long but good branch, except for one annoying detail that you'll find below.
.
Could you document Poller to set the right expectations with the reader? The "flyweight" approach in particular looks a bit confusing. My understanding is that the Poller gives you a response and then you need to ask that same Poller whether that response means that you're done... I think I understand why it's done that way (it allows for an immutable poller) but it would help to have it stated.
I like the separation between the Poller interface and the internal implementation. Exposing the x509Response doesn't really fit in though. My recollection is that we had meant that to be internal to gwacl, given the duplication with http.Response, and now x509Response is no longer exported. This part needs fixing, I think — though not necessarily in the same branch.
.
Well done for documenting the pointless panic at the end of StartPolling. I guess in the future the compiler will get smarter, detect that the loop never exits, and complain that the panic() is unreachable. The compiler giveth, and then the compiler taketh away.
.
On reflection, maybe StartPolling() isn't quite the right name, in that it implies that it won't do the actual polling.
.
Typo in the documentation for operationPoller: "A object."
By the way, I'm not sure if documentation for a type should also begin with its name.
.
The documentation for operationPoller
.
In operationPoller
.
I like the tests. Excellent comment in TestStartPollin
.
In TestStartOperat
.
Jeroen
- 63. By Raphaël Badin
-
Review comments.
Raphaël Badin (rvb) wrote : | # |
Thanks for the review.
>
> Could you document Poller to set the right expectations with the reader? The
> "flyweight" approach in particular looks a bit confusing. My understanding is
> that the Poller gives you a response and then you need to ask that same Poller
> whether that response means that you're done... I think I understand why it's
> done that way (it allows for an immutable poller) but it would help to have it
> stated.
Added a comments.
> I like the separation between the Poller interface and the internal
> implementation. Exposing the x509Response doesn't really fit in though. My
> recollection is that we had meant that to be internal to gwacl, given the
> duplication with http.Response, and now x509Response is no longer exported.
> This part needs fixing, I think — though not necessarily in the same branch.
You're right, now that x509Response is not exposed, the method in the poller interface should not be exposed either. Same for startPolling which is more of an internal utility method used by more concrete implementations such as StartOperationP
> On reflection, maybe StartPolling() isn't quite the right name, in that it
> implies that it won't do the actual polling.
Hum, I don't really have a better idea right now so I'll land this as is and we will see if we come up with a better name later.
> Typo in the documentation for operationPoller: "A object."
Fixed.
> The documentation for operationPoller
> lost in trying to describe the implementation. Document what they mean to the
> caller, not how they do their work! As a caller, I really don't care whether
> Poll() issues a GET request or a HEAD request — gwacl's exact purpose is to
> hide that sort of thing from me. What I care about as a caller is primarily:
> that I need to call these methods in order to wait for my prior request to
> complete and secondarily: that it makes outgoing requests and that it blocks.
This is really internal stuff, no user will ever call these methods directly, hence the focus on explaining what the methods do internally.
> In operationPoller
> that need an error check.
Indeed, I've left it as is, we will fix this in another branch.
> In TestStartOperat
> comment and "Succeeded" is misspelled twice as "Succeded" — which may be
> working just because it's consistent, but it's late for me.
oops, fixed.
Thanks again for the review.
- 64. By Raphaël Badin
-
Merge trunk, fix conflicts.
Julian Edwards (julian-edwards) wrote : | # |
On Wednesday 27 Mar 2013 14:09:17 you wrote:
> > Oh, and ESPECIALLY don't arbitrarily switch genders for the same person¹.
> > Did somebody make you a bet that it would set me off? :)
>
> Well, I guess I know which button to push if I want to set you off now :).
There are SO many to choose.
Julian Edwards (julian-edwards) wrote : | # |
On Wednesday 27 Mar 2013 17:06:24 you wrote:
> > On reflection, maybe StartPolling() isn't quite the right name, in that it
> > implies that it won't do the actual polling.
>
> Hum, I don't really have a better idea right now so I'll land this as is and
> we will see if we come up with a better name later.
There is a certain school of thought that says this method should not even be
exported, and that the API should just block (with a timeout) until the async
operation completes.
Jeroen T. Vermeulen (jtv) wrote : | # |
Yes, it is the fact that Poller is exported that made me think that it was to be part of gwacl's external API.
Preview Diff
1 | === modified file 'managementapi.go' |
2 | --- managementapi.go 2013-03-26 06:50:50 +0000 |
3 | +++ managementapi.go 2013-03-27 17:10:31 +0000 |
4 | @@ -32,7 +32,7 @@ |
5 | return nil, err |
6 | } |
7 | hostedServices := HostedServiceDescriptorList{} |
8 | - xml.Unmarshal(res, &hostedServices) |
9 | + xml.Unmarshal(res.Body, &hostedServices) |
10 | return hostedServices.HostedServices, nil |
11 | } |
12 | |
13 | @@ -44,7 +44,7 @@ |
14 | return nil, err |
15 | } |
16 | hostedService := HostedService{} |
17 | - xml.Unmarshal(res, &hostedService) |
18 | + xml.Unmarshal(res.Body, &hostedService) |
19 | return &hostedService, nil |
20 | } |
21 | |
22 | |
23 | === modified file 'managementapi_test.go' |
24 | --- managementapi_test.go 2013-03-26 09:54:07 +0000 |
25 | +++ managementapi_test.go 2013-03-27 17:10:31 +0000 |
26 | @@ -48,12 +48,16 @@ |
27 | return &recordedRequests |
28 | } |
29 | |
30 | -// checkRequest asserts that the given slice contains one request, with the |
31 | +// checkOneRequest asserts that the given slice contains one request, with the |
32 | // given characteristics. |
33 | -func checkRequest(c *C, recordedRequests *[]*x509Request, URL string, payload []byte, Method string) { |
34 | +func checkOneRequest(c *C, recordedRequests *[]*x509Request, URL string, payload []byte, Method string) { |
35 | requests := *recordedRequests |
36 | c.Assert(len(requests), Equals, 1) |
37 | request := requests[0] |
38 | + checkRequest(c, request, URL, payload, Method) |
39 | +} |
40 | + |
41 | +func checkRequest(c *C, request *x509Request, URL string, payload []byte, Method string) { |
42 | c.Assert(request.URL, Equals, URL) |
43 | c.Assert(string(request.Payload), Equals, string(payload)) |
44 | c.Assert(request.Method, Equals, Method) |
45 | @@ -61,33 +65,33 @@ |
46 | |
47 | func (suite *managementAPISuite) TestListHostedServiceDescriptors(c *C) { |
48 | api := suite.makeAPI(c) |
49 | - recordedRequests := suite.setUpDispatcher() |
50 | + recordedRequests := setUpDispatcher("operationID") |
51 | |
52 | _, err := api.ListHostedServiceDescriptors() |
53 | |
54 | c.Assert(err, IsNil) |
55 | expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices" |
56 | - checkRequest(c, recordedRequests, expectedURL, []byte{}, "GET") |
57 | + checkOneRequest(c, recordedRequests, expectedURL, []byte{}, "GET") |
58 | } |
59 | |
60 | // TODO test that ListHostedServiceDescriptors parses the structures correctly |
61 | |
62 | func (suite *managementAPISuite) TestLoadHostedService(c *C) { |
63 | api := suite.makeAPI(c) |
64 | - recordedRequests := suite.setUpDispatcher() |
65 | + recordedRequests := setUpDispatcher("operationID") |
66 | serviceName := "serviceName" |
67 | _, err := api.LoadHostedService(serviceName) |
68 | |
69 | c.Assert(err, IsNil) |
70 | expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + serviceName + "?embed-detail=true" |
71 | - checkRequest(c, recordedRequests, expectedURL, []byte{}, "GET") |
72 | + checkOneRequest(c, recordedRequests, expectedURL, []byte{}, "GET") |
73 | } |
74 | |
75 | // TODO test that TestLoadHostedService parses the structures correctly |
76 | |
77 | func (suite *managementAPISuite) TestAddHostedService(c *C) { |
78 | api := suite.makeAPI(c) |
79 | - recordedRequests := suite.setUpDispatcher() |
80 | + recordedRequests := setUpDispatcher("operationID") |
81 | createHostedService := NewCreateHostedServiceWithLocation("testName", "testLabel", "East US") |
82 | err := api.AddHostedService(createHostedService) |
83 | |
84 | @@ -95,23 +99,23 @@ |
85 | expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices" |
86 | expectedPayload, err := marshalXML(createHostedService) |
87 | c.Assert(err, IsNil) |
88 | - checkRequest(c, recordedRequests, expectedURL, expectedPayload, "POST") |
89 | + checkOneRequest(c, recordedRequests, expectedURL, expectedPayload, "POST") |
90 | } |
91 | |
92 | func (suite *managementAPISuite) TestDeleteHostedService(c *C) { |
93 | api := suite.makeAPI(c) |
94 | - recordedRequests := suite.setUpDispatcher() |
95 | + recordedRequests := setUpDispatcher("operationID") |
96 | hostedServiceName := "testName" |
97 | err := api.DeleteHostedService(hostedServiceName) |
98 | |
99 | c.Assert(err, IsNil) |
100 | expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + hostedServiceName |
101 | - checkRequest(c, recordedRequests, expectedURL, []byte{}, "DELETE") |
102 | + checkOneRequest(c, recordedRequests, expectedURL, []byte{}, "DELETE") |
103 | } |
104 | |
105 | func (suite *managementAPISuite) TestAddDeployment(c *C) { |
106 | api := suite.makeAPI(c) |
107 | - recordedRequests := suite.setUpDispatcher() |
108 | + recordedRequests := setUpDispatcher("operationID") |
109 | serviceName := "serviceName" |
110 | configurationSet := NewLinuxProvisioningConfiguration("testHostname12345", "test", "test123#@!", false) |
111 | OSVirtualHardDisk := NewOSVirtualHardDisk("hostCaching", "diskLabel", "diskName", "http://mediaLink", "sourceImageName") |
112 | @@ -123,7 +127,7 @@ |
113 | expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + serviceName + "/deployments" |
114 | expectedPayload, err := marshalXML(deployment) |
115 | c.Assert(err, IsNil) |
116 | - checkRequest(c, recordedRequests, expectedURL, expectedPayload, "POST") |
117 | + checkOneRequest(c, recordedRequests, expectedURL, expectedPayload, "POST") |
118 | } |
119 | |
120 | func (suite *managementAPISuite) TestAddStorageAccount(c *C) { |
121 | @@ -139,12 +143,12 @@ |
122 | expectedURL := AZURE_URL + api.session.subscriptionId + "/services/storageservices" |
123 | expectedPayload, err := marshalXML(cssi) |
124 | c.Assert(err, IsNil) |
125 | - checkRequest(c, &recordedRequests, expectedURL, expectedPayload, "POST") |
126 | + checkOneRequest(c, &recordedRequests, expectedURL, expectedPayload, "POST") |
127 | } |
128 | |
129 | func (suite *managementAPISuite) TestPerformNodeOperation(c *C) { |
130 | api := suite.makeAPI(c) |
131 | - recordedRequests := suite.setUpDispatcher() |
132 | + recordedRequests := setUpDispatcher("operationID") |
133 | serviceName := "serviceName" |
134 | deploymentName := "deploymentName" |
135 | roleName := "roleName" |
136 | @@ -155,5 +159,5 @@ |
137 | expectedURL := AZURE_URL + api.session.subscriptionId + "/services/hostedservices/" + serviceName + "/deployments/" + deploymentName + "/roleinstances/" + roleName + "/Operations" |
138 | expectedPayload, err := marshalXML(operation) |
139 | c.Assert(err, IsNil) |
140 | - checkRequest(c, recordedRequests, expectedURL, expectedPayload, "POST") |
141 | + checkOneRequest(c, recordedRequests, expectedURL, expectedPayload, "POST") |
142 | } |
143 | |
144 | === added file 'poller.go' |
145 | --- poller.go 1970-01-01 00:00:00 +0000 |
146 | +++ poller.go 2013-03-27 17:10:31 +0000 |
147 | @@ -0,0 +1,92 @@ |
148 | +// Copyright 2013 Canonical Ltd. This software is licensed under the |
149 | +// GNU Lesser General Public License version 3 (see the file COPYING). |
150 | + |
151 | +package gwacl |
152 | + |
153 | +import ( |
154 | + "encoding/xml" |
155 | + "time" |
156 | +) |
157 | + |
158 | +// Generic poller interface/methods. |
159 | + |
160 | +// A Poller exposes two methods to query a remote server and decide when |
161 | +// the response given by the server means that the polling is finished. |
162 | +type Poller interface { |
163 | + poll() (*x509Response, error) |
164 | + isDone(*x509Response) bool |
165 | +} |
166 | + |
167 | +// startPolling calls the poll() method of the given 'poller' object every |
168 | +// 'interval' until poller.isDone() return true. |
169 | +func startPolling(poller Poller, interval time.Duration) (*x509Response, error) { |
170 | + // TODO: Add a timeout so that polling won't continue forever if isDone() |
171 | + // always returns false. |
172 | + ticker := time.Tick(interval) |
173 | + for _ = range ticker { |
174 | + response, err := poller.poll() |
175 | + if err != nil { |
176 | + return nil, err |
177 | + } |
178 | + if poller.isDone(response) { |
179 | + return response, nil |
180 | + } |
181 | + } |
182 | + // This code cannot be reached but Go insists on having a return or a panic |
183 | + // statement at the end of this method. Sigh. |
184 | + panic("invalid poller state!") |
185 | +} |
186 | + |
187 | +// Operation poller structs/methods. |
188 | + |
189 | +// StartOperationPolling calls startPolling on the given arguments and converts |
190 | +// the returned object into an *Operation. |
191 | +func StartOperationPolling(poller Poller, interval time.Duration) (*Operation, error) { |
192 | + response, err := startPolling(poller, interval) |
193 | + if err != nil { |
194 | + return nil, err |
195 | + } |
196 | + operation := Operation{} |
197 | + xml.Unmarshal(response.Body, &operation) |
198 | + return &operation, nil |
199 | +} |
200 | + |
201 | +// operationPoller is an object implementing the Poller interface, used to |
202 | +// poll the Window Azure server until the operation referenced by the given |
203 | +// operationID is completed. |
204 | +type operationPoller struct { |
205 | + api *ManagementAPI |
206 | + operationID string |
207 | +} |
208 | + |
209 | +var _ Poller = &operationPoller{} |
210 | + |
211 | +// NewOperationPoller returns a poller object associated with the given |
212 | +// management API object and the given operationID. It can track (by polling |
213 | +// the server) the status of the operation associated with the provided |
214 | +// operationID string. |
215 | +func NewOperationPoller(api *ManagementAPI, operationID string) Poller { |
216 | + return operationPoller{api: api, operationID: operationID} |
217 | +} |
218 | + |
219 | +// Poll issues a blocking request to microsoft Azure to fetch the information |
220 | +// related to the operation associated with the poller. |
221 | +func (poller operationPoller) poll() (*x509Response, error) { |
222 | + URI := "operations/" + poller.operationID |
223 | + return poller.api.session.get(URI) |
224 | +} |
225 | + |
226 | +// IsDone returns true if the given response has a status code indicating |
227 | +// success and if the returned XML response corresponds to a valid Operation |
228 | +// with a status indicating that the operation is completed. |
229 | +func (poller operationPoller) isDone(response *x509Response) bool { |
230 | + // TODO: Add a timeout so that polling won't continue forever if the |
231 | + // server cannot be reached. |
232 | + if response.StatusCode >= 200 && response.StatusCode < 300 { |
233 | + operation := Operation{} |
234 | + xml.Unmarshal(response.Body, &operation) |
235 | + status := operation.Status |
236 | + return (status != "" && status != InProgressOperationStatus) |
237 | + } |
238 | + return false |
239 | +} |
240 | |
241 | === added file 'poller_test.go' |
242 | --- poller_test.go 1970-01-01 00:00:00 +0000 |
243 | +++ poller_test.go 2013-03-27 17:10:31 +0000 |
244 | @@ -0,0 +1,187 @@ |
245 | +// Copyright 2013 Canonical Ltd. This software is licensed under the |
246 | +// GNU Lesser General Public License version 3 (see the file COPYING). |
247 | + |
248 | +package gwacl |
249 | + |
250 | +import ( |
251 | + "fmt" |
252 | + . "launchpad.net/gocheck" |
253 | + "launchpad.net/gwacl/dedent" |
254 | + "net/http" |
255 | + "time" |
256 | +) |
257 | + |
258 | +type pollerSuite struct{} |
259 | + |
260 | +var _ = Suite(&pollerSuite{}) |
261 | + |
262 | +func (suite *pollerSuite) makeAPI(c *C) *ManagementAPI { |
263 | + subscriptionId := "subscriptionId" |
264 | + subscriptionId = subscriptionId |
265 | + api, err := NewManagementAPI(subscriptionId, "certFile") |
266 | + c.Assert(err, IsNil) |
267 | + return api |
268 | +} |
269 | + |
270 | +// testPoller is a struct which implements the Poller interface. It records |
271 | +// the number of calls to testPoller.Poll(). |
272 | +type testPoller struct { |
273 | + recordedPollCalls int |
274 | + notDoneCalls int |
275 | + errorCalls int |
276 | +} |
277 | + |
278 | +var testPollerResponse = x509Response{} |
279 | +var testPollerError = fmt.Errorf("Test error") |
280 | + |
281 | +// newTestPoller return a pointer to a testPoller object that will return |
282 | +// false when IsDone() will be called 'notDoneCalls' number of times and that |
283 | +// will error when Poll() will be called 'errorCalls' number of times. |
284 | +func newTestPoller(notDoneCalls int, errorCalls int) *testPoller { |
285 | + return &testPoller{0, notDoneCalls, errorCalls} |
286 | +} |
287 | + |
288 | +func (poller *testPoller) poll() (*x509Response, error) { |
289 | + if poller.errorCalls > 0 { |
290 | + poller.errorCalls -= 1 |
291 | + return nil, testPollerError |
292 | + } |
293 | + poller.recordedPollCalls += 1 |
294 | + return &testPollerResponse, nil |
295 | +} |
296 | + |
297 | +func (poller *testPoller) isDone(response *x509Response) bool { |
298 | + if poller.notDoneCalls == 0 { |
299 | + return true |
300 | + } |
301 | + poller.notDoneCalls = poller.notDoneCalls - 1 |
302 | + return false |
303 | +} |
304 | + |
305 | +func (suite *pollerSuite) TeststartPollingReturnsError(c *C) { |
306 | + poller := newTestPoller(0, 1) |
307 | + response, err := startPolling(poller, time.Millisecond) |
308 | + |
309 | + c.Assert(err, Equals, testPollerError) |
310 | + c.Assert(response, IsNil) |
311 | +} |
312 | + |
313 | +func (suite *pollerSuite) TeststartPollingRetries(c *C) { |
314 | + poller := newTestPoller(2, 0) |
315 | + response, err := startPolling(poller, time.Millisecond) |
316 | + |
317 | + c.Assert(err, IsNil) |
318 | + c.Assert(response, Equals, &testPollerResponse) |
319 | + // Poll() has been called 3 times: two calls for which IsDone() returned |
320 | + // false and one for which IsDone() return true. |
321 | + c.Assert(poller.recordedPollCalls, Equals, 3) |
322 | +} |
323 | + |
324 | +func (suite *pollerSuite) TestNewOperationPoller(c *C) { |
325 | + api := suite.makeAPI(c) |
326 | + operationID := "operationID" |
327 | + |
328 | + poller := NewOperationPoller(api, operationID) |
329 | + |
330 | + operationPollerInstance := poller.(operationPoller) |
331 | + c.Check(operationPollerInstance.api, Equals, api) |
332 | + c.Check(operationPollerInstance.operationID, Equals, operationID) |
333 | +} |
334 | + |
335 | +func (suite *pollerSuite) TestOperationPollerPoll(c *C) { |
336 | + api := suite.makeAPI(c) |
337 | + operationID := "operationID" |
338 | + poller := NewOperationPoller(api, operationID) |
339 | + recordedRequests := setUpDispatcher("operationID") |
340 | + |
341 | + _, err := poller.poll() |
342 | + |
343 | + c.Assert(err, IsNil) |
344 | + expectedURL := AZURE_URL + api.session.subscriptionId + "/operations/" + operationID |
345 | + checkOneRequest(c, recordedRequests, expectedURL, []byte{}, "GET") |
346 | +} |
347 | + |
348 | +var operationXMLTemplate = dedent.Dedent(` |
349 | +<?xml version="1.0" encoding="utf-8"?> |
350 | + <Operation xmlns="http://schemas.microsoft.com/windowsazure"> |
351 | + <ID>bogus-request-id</ID> |
352 | + <Status>%s</Status> |
353 | + </Operation> |
354 | +`) |
355 | + |
356 | +func (suite *pollerSuite) TestOperationPollerIsDoneReturnsTrueIfOperationDone(c *C) { |
357 | + poller := NewOperationPoller(suite.makeAPI(c), "operationID") |
358 | + operationStatuses := []string{"Succeeded", "Failed"} |
359 | + for _, operationStatus := range operationStatuses { |
360 | + body := fmt.Sprintf(operationXMLTemplate, operationStatus) |
361 | + response := x509Response{ |
362 | + Body: []byte(body), |
363 | + StatusCode: http.StatusOK, |
364 | + } |
365 | + |
366 | + isDone := poller.isDone(&response) |
367 | + c.Assert(isDone, Equals, true) |
368 | + } |
369 | +} |
370 | + |
371 | +func (suite *pollerSuite) TestOperationPollerIsDoneReturnsFalse(c *C) { |
372 | + poller := NewOperationPoller(suite.makeAPI(c), "operationID") |
373 | + notDoneResponses := []x509Response{ |
374 | + // 'InProgress' response. |
375 | + x509Response{ |
376 | + Body: []byte(fmt.Sprintf(operationXMLTemplate, "InProgress")), |
377 | + StatusCode: http.StatusOK, |
378 | + }, |
379 | + // Invalid XML content. |
380 | + x509Response{ |
381 | + Body: []byte("invalid XML"), |
382 | + StatusCode: http.StatusOK, |
383 | + }, |
384 | + // Error statuses. |
385 | + x509Response{StatusCode: http.StatusNotFound}, |
386 | + x509Response{StatusCode: http.StatusBadRequest}, |
387 | + x509Response{StatusCode: http.StatusInternalServerError}, |
388 | + } |
389 | + for _, response := range notDoneResponses { |
390 | + isDone := poller.isDone(&response) |
391 | + c.Assert(isDone, Equals, false) |
392 | + } |
393 | +} |
394 | + |
395 | +func (suite *pollerSuite) TestStartOperationPollingRetries(c *C) { |
396 | + // This is an end-to-end test of the operation poller; there is a certain |
397 | + // amount of duplication with the unit tests for startPolling() but |
398 | + // it's probably worth it to thoroughly test StartOperationPolling(). |
399 | + |
400 | + // Fake 2 responses in sequence: a 'InProgress' response and then a |
401 | + // 'Succeeded' response. |
402 | + firstResponse := DispatcherResponse{ |
403 | + response: &x509Response{ |
404 | + Body: []byte(fmt.Sprintf(operationXMLTemplate, "InProgress")), |
405 | + StatusCode: http.StatusOK, |
406 | + }, |
407 | + errorObject: nil} |
408 | + secondResponse := DispatcherResponse{ |
409 | + response: &x509Response{ |
410 | + Body: []byte(fmt.Sprintf(operationXMLTemplate, "Succeeded")), |
411 | + StatusCode: http.StatusOK, |
412 | + }, |
413 | + errorObject: nil} |
414 | + responses := []DispatcherResponse{firstResponse, secondResponse} |
415 | + rigPreparedResponseDispatcher(responses) |
416 | + recordedRequests := make([]*x509Request, 0) |
417 | + rigRecordingDispatcher(&recordedRequests) |
418 | + |
419 | + // Setup poller and start it. |
420 | + operationID := "operationID" |
421 | + poller := NewOperationPoller(suite.makeAPI(c), operationID) |
422 | + response, err := startPolling(poller, time.Millisecond) |
423 | + |
424 | + c.Assert(err, IsNil) |
425 | + c.Assert(response, Equals, secondResponse.response) |
426 | + operationPollerInstance := poller.(operationPoller) |
427 | + expectedURL := AZURE_URL + operationPollerInstance.api.session.subscriptionId + "/operations/" + operationID |
428 | + c.Assert(len(recordedRequests), Equals, 2) |
429 | + checkRequest(c, recordedRequests[0], expectedURL, []byte{}, "GET") |
430 | + checkRequest(c, recordedRequests[1], expectedURL, []byte{}, "GET") |
431 | +} |
432 | |
433 | === modified file 'test_helpers.go' |
434 | --- test_helpers.go 2013-03-21 02:14:39 +0000 |
435 | +++ test_helpers.go 2013-03-27 17:10:31 +0000 |
436 | @@ -6,6 +6,7 @@ |
437 | import ( |
438 | "fmt" |
439 | "math/rand" |
440 | + "net/http" |
441 | "strings" |
442 | "time" |
443 | ) |
444 | @@ -181,6 +182,69 @@ |
445 | ExtendedProperties: ExtendedProperties} |
446 | } |
447 | |
448 | +// rigRecordingDispatcher sets up a request dispatcher that records incoming |
449 | +// requests by appending them to *record. It returns the result of whatever |
450 | +// dispatcher was already active. |
451 | +// If you also want the dispatcher to return a particular result, rig it for |
452 | +// that result first (using one of the other rig...Dispatcher functions) and |
453 | +// then chain the recording dispatcher in front of it. |
454 | +func rigRecordingDispatcher(record *[]*x509Request) { |
455 | + previousDispatcher := _X509Dispatcher |
456 | + _X509Dispatcher = func(session *x509Session, request *x509Request) (*x509Response, error) { |
457 | + *record = append(*record, request) |
458 | + return previousDispatcher(session, request) |
459 | + } |
460 | +} |
461 | + |
462 | +// rigFixedResponseDispatcher sets up a request dispatcher that always returns |
463 | +// a prepared response. |
464 | +func rigFixedResponseDispatcher(response *x509Response) { |
465 | + _X509Dispatcher = func(*x509Session, *x509Request) (*x509Response, error) { |
466 | + return response, nil |
467 | + } |
468 | +} |
469 | + |
470 | +// rigFailingDispatcher sets up a request dispatcher that returns a given |
471 | +// error. |
472 | +func rigFailingDispatcher(failure error) { |
473 | + _X509Dispatcher = func(*x509Session, *x509Request) (*x509Response, error) { |
474 | + return nil, failure |
475 | + } |
476 | +} |
477 | + |
478 | +type DispatcherResponse struct { |
479 | + response *x509Response |
480 | + errorObject error |
481 | +} |
482 | + |
483 | +// rigPreparedResponseDispatcher sets up a request dispatcher that all the provided |
484 | +// reponses, one after the other each time the dispatcher is called. |
485 | +func rigPreparedResponseDispatcher(responses []DispatcherResponse) { |
486 | + index := 0 |
487 | + _X509Dispatcher = func(*x509Session, *x509Request) (*x509Response, error) { |
488 | + response := responses[index] |
489 | + index += 1 |
490 | + return response.response, response.errorObject |
491 | + } |
492 | +} |
493 | + |
494 | +// setUpDispatcher sets up a request dispatcher that: |
495 | +// - records requests |
496 | +// - returns empty responses |
497 | +func setUpDispatcher(operationID string) *[]*x509Request { |
498 | + header := http.Header{} |
499 | + header.Set("X-Ms-Request-Id", operationID) |
500 | + fixedResponse := x509Response{ |
501 | + StatusCode: http.StatusOK, |
502 | + Body: []byte{}, |
503 | + Header: header, |
504 | + } |
505 | + rigFixedResponseDispatcher(&fixedResponse) |
506 | + recordedRequests := make([]*x509Request, 0) |
507 | + rigRecordingDispatcher(&recordedRequests) |
508 | + return &recordedRequests |
509 | +} |
510 | + |
511 | func init() { |
512 | // Staggeringly, rand will produce the same numbers from the start of |
513 | // program invocation unless you seed it ... |
514 | |
515 | === modified file 'x509session.go' |
516 | --- x509session.go 2013-03-27 07:16:45 +0000 |
517 | +++ x509session.go 2013-03-27 17:10:31 +0000 |
518 | @@ -43,7 +43,7 @@ |
519 | // It returns the response body and/or an error. If the error is a |
520 | // ServerError, the returned body will be the one received from the server. |
521 | // For any other kind of error, the returned body will be nil. |
522 | -func (session *x509Session) get(url string) ([]byte, error) { |
523 | +func (session *x509Session) get(url string) (*x509Response, error) { |
524 | fullUrl := composeURL(url, session.subscriptionId) |
525 | request := newX509RequestGET(fullUrl) |
526 | response, err := _X509Dispatcher(session, request) |
527 | @@ -51,14 +51,14 @@ |
528 | return nil, err |
529 | } |
530 | err = session.getServerError(response.StatusCode, response.Body, "GET request failed") |
531 | - return response.Body, err |
532 | + return response, err |
533 | } |
534 | |
535 | // post performs a POST request to the Azure management API. |
536 | // It returns the response body and/or an error. If the error is a |
537 | // ServerError, the returned body will be the one received from the server. |
538 | // For any other kind of error, the returned body will be nil. |
539 | -func (session *x509Session) post(url string, body []byte, contentType string) ([]byte, error) { |
540 | +func (session *x509Session) post(url string, body []byte, contentType string) (*x509Response, error) { |
541 | fullUrl := composeURL(url, session.subscriptionId) |
542 | request := newX509RequestPOST(fullUrl, body, contentType) |
543 | response, err := _X509Dispatcher(session, request) |
544 | @@ -66,7 +66,7 @@ |
545 | return nil, err |
546 | } |
547 | err = session.getServerError(response.StatusCode, response.Body, "POST request failed") |
548 | - return response.Body, err |
549 | + return response, err |
550 | } |
551 | |
552 | // delete performs a DELETE request to the Azure management API. |
553 | @@ -81,12 +81,12 @@ |
554 | } |
555 | |
556 | // put performs a PUT request to the Azure management API. |
557 | -func (session *x509Session) put(url string, body []byte) error { |
558 | +func (session *x509Session) put(url string, body []byte) (*x509Response, error) { |
559 | fullUrl := composeURL(url, session.subscriptionId) |
560 | request := newX509RequestPUT(fullUrl, body) |
561 | response, err := _X509Dispatcher(session, request) |
562 | if err != nil { |
563 | - return err |
564 | + return nil, err |
565 | } |
566 | - return session.getServerError(response.StatusCode, response.Body, "POST request failed") |
567 | + return response, session.getServerError(response.StatusCode, response.Body, "POST request failed") |
568 | } |
569 | |
570 | === modified file 'x509session_test.go' |
571 | --- x509session_test.go 2013-03-27 16:34:04 +0000 |
572 | +++ x509session_test.go 2013-03-27 17:10:31 +0000 |
573 | @@ -44,36 +44,6 @@ |
574 | |
575 | var _ = Suite(&x509SessionSuite{}) |
576 | |
577 | -// rigRecordingDispatcher sets up a request dispatcher that records incoming |
578 | -// requests by appending them to *record. It returns the result of whatever |
579 | -// dispatcher was already active. |
580 | -// If you also want the dispatcher to return a particular result, rig it for |
581 | -// that result first (using one of the other rig...Dispatcher functions) and |
582 | -// then chain the recording dispatcher in front of it. |
583 | -func rigRecordingDispatcher(record *[]*x509Request) { |
584 | - previousDispatcher := _X509Dispatcher |
585 | - _X509Dispatcher = func(session *x509Session, request *x509Request) (*x509Response, error) { |
586 | - *record = append(*record, request) |
587 | - return previousDispatcher(session, request) |
588 | - } |
589 | -} |
590 | - |
591 | -// rigFixedResponseDispatcher sets up a request dispatcher that always returns |
592 | -// a prepared response. |
593 | -func rigFixedResponseDispatcher(response *x509Response) { |
594 | - _X509Dispatcher = func(*x509Session, *x509Request) (*x509Response, error) { |
595 | - return response, nil |
596 | - } |
597 | -} |
598 | - |
599 | -// rigFailingDispatcher sets up a request dispatcher that returns a given |
600 | -// error. |
601 | -func rigFailingDispatcher(failure error) { |
602 | - _X509Dispatcher = func(*x509Session, *x509Request) (*x509Response, error) { |
603 | - return nil, failure |
604 | - } |
605 | -} |
606 | - |
607 | // Create a cert and pem file in a temporary dir in /tmp and return the |
608 | // names of the files. The caller is responsible for cleaning up the files. |
609 | func makeX509Certificate() (string, string) { |
610 | @@ -198,14 +168,14 @@ |
611 | recordedRequests := make([]*x509Request, 0) |
612 | rigRecordingDispatcher(&recordedRequests) |
613 | |
614 | - receivedBody, err := session.get(uri) |
615 | + receivedResponse, err := session.get(uri) |
616 | c.Assert(err, IsNil) |
617 | |
618 | c.Assert(len(recordedRequests), Equals, 1) |
619 | request := recordedRequests[0] |
620 | c.Check(request.URL, Equals, AZURE_URL+subscriptionID+"/"+uri) |
621 | c.Check(request.Method, Equals, "GET") |
622 | - c.Check(receivedBody, DeepEquals, fixedResponse.Body) |
623 | + c.Check(*receivedResponse, DeepEquals, fixedResponse) |
624 | } |
625 | |
626 | func (suite *x509SessionSuite) TestGetReportsClientSideError(c *C) { |
627 | @@ -228,12 +198,12 @@ |
628 | } |
629 | rigFixedResponseDispatcher(&fixedResponse) |
630 | |
631 | - body, err := session.get("/fail") |
632 | + response, err := session.get("/fail") |
633 | c.Assert(err, NotNil) |
634 | |
635 | serverError := err.(*ServerError) |
636 | c.Check(serverError.StatusCode(), Equals, fixedResponse.StatusCode) |
637 | - c.Check(body, DeepEquals, fixedResponse.Body) |
638 | + c.Check(*response, DeepEquals, fixedResponse) |
639 | } |
640 | |
641 | func (suite *x509SessionSuite) TestPostIssuesRequest(c *C) { |
642 | @@ -252,7 +222,7 @@ |
643 | recordedRequests := make([]*x509Request, 0) |
644 | rigRecordingDispatcher(&recordedRequests) |
645 | |
646 | - receivedBody, err := session.post(uri, requestBody, requestContentType) |
647 | + receivedResponse, err := session.post(uri, requestBody, requestContentType) |
648 | c.Assert(err, IsNil) |
649 | |
650 | c.Assert(len(recordedRequests), Equals, 1) |
651 | @@ -261,7 +231,7 @@ |
652 | c.Check(request.Method, Equals, "POST") |
653 | c.Check(request.ContentType, Equals, requestContentType) |
654 | c.Check(request.Payload, DeepEquals, requestBody) |
655 | - c.Check(receivedBody, DeepEquals, fixedResponse.Body) |
656 | + c.Check(*receivedResponse, DeepEquals, fixedResponse) |
657 | } |
658 | |
659 | func (suite *x509SessionSuite) TestPostReportsClientSideError(c *C) { |
660 | @@ -284,12 +254,12 @@ |
661 | } |
662 | rigFixedResponseDispatcher(&fixedResponse) |
663 | |
664 | - body, err := session.post("/fail", []byte("request body"), "contentType") |
665 | + reponse, err := session.post("/fail", []byte("request body"), "contentType") |
666 | c.Assert(err, NotNil) |
667 | |
668 | serverError := err.(*ServerError) |
669 | c.Check(serverError.StatusCode(), Equals, fixedResponse.StatusCode) |
670 | - c.Check(body, DeepEquals, fixedResponse.Body) |
671 | + c.Check(*reponse, DeepEquals, fixedResponse) |
672 | } |
673 | |
674 | func (suite *x509SessionSuite) TestDeleteIssuesRequest(c *C) { |
675 | @@ -326,7 +296,7 @@ |
676 | recordedRequests := make([]*x509Request, 0) |
677 | rigRecordingDispatcher(&recordedRequests) |
678 | |
679 | - err = session.put(uri, requestBody) |
680 | + _, err = session.put(uri, requestBody) |
681 | c.Assert(err, IsNil) |
682 | |
683 | c.Assert(len(recordedRequests), Equals, 1) |
684 | |
685 | === modified file 'xmlobjects.go' |
686 | --- xmlobjects.go 2013-03-27 07:16:45 +0000 |
687 | +++ xmlobjects.go 2013-03-27 17:10:31 +0000 |
688 | @@ -520,3 +520,17 @@ |
689 | func (g *GetBlockList) Deserialize(data []byte) error { |
690 | return xml.Unmarshal(data, g) |
691 | } |
692 | + |
693 | +// |
694 | +// Operation Services bits |
695 | +// |
696 | + |
697 | +var InProgressOperationStatus = "InProgress" |
698 | + |
699 | +type Operation struct { |
700 | + ID string `xml:"ID"` |
701 | + Status string `xml:"Status"` |
702 | + HttpStatusCode int `xml:"HttpStatusCode"` |
703 | + ErrorCode string `xml:"Error>Code"` |
704 | + ErrorMessage string `xml:"Error>Message"` |
705 | +} |
Please don't refer to a generic "the caller" as "she"! It's especially confusing because unlike "une prick," a caller is grammatically masculine.
English has a better way of dealing with indeterminate genders: use "they," with the corresponding plural verbs. Unlike other languages, English has no problem with switching conjugations halfway through a sentence for this purpose — "If the customer _has_ a complaint, remember to say that _they are_ right."
(Also, "wants not to block" is geek-speak. In real English you'd say "does not want to block," or perhaps "wants to avoid blocking.")