Merge lp:~allenap/gwacl/list-instances into lp:gwacl
- list-instances
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Gavin Panella |
Approved revision: | 131 |
Merged at revision: | 126 |
Proposed branch: | lp:~allenap/gwacl/list-instances |
Merge into: | lp:gwacl |
Diff against target: |
540 lines (+256/-31) 5 files modified
management.go (+24/-0) management_base_test.go (+31/-31) management_test.go (+180/-0) testhelpers_x509dispatch.go (+13/-0) xmlobjects.go (+8/-0) |
To merge this branch: | bzr merge lp:~allenap/gwacl/list-instances |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jeroen T. Vermeulen (community) | Approve | ||
Review via email: mp+170860@code.launchpad.net |
Commit message
New management API meta-method to list all instances.
Description of the change
Jeroen T. Vermeulen (jtv) wrote : | # |
Jeroen T. Vermeulen (jtv) wrote : | # |
Ho hum. I said Approved, but evidently I didn't tell the voting system.
- 129. By Gavin Panella
-
Fix comment for rigRecordingPre
paredResponseDi spatcher, and simplify response handling. - 130. By Gavin Panella
-
Use NotNil instead of Not(IsNil).
- 131. By Gavin Panella
-
Ensure that error messages are congruent with the expected error.
Gavin Panella (allenap) wrote : | # |
> It's depressing that it's all so much work, that the code gets so verbose.
> And that makes me hesitant to suggest further improvements that involve yet
> more work.
Indeed. I really wanted to stub out the calls to ListHostedServi
and GetHostedServic
> Nevertheless, here's what I came up with:
>
> 1. The way rigRecordingPre
> was not entirely obvious to me, what with the combination of slices, closures,
> and the returned pointer to a nested function. A comment might help.
I originally copied the existing recording dispatcher which uses an
index. I tried out ways of popping from a slice in Go, and didn't
really mean to commit this.
>
> 2. There's a NotNil checker, shorthand for Not(IsNil).
Ta, changed.
>
> 3. It'd be good to test at least *something* about the errors you get back,
> not just the fact that you get an error. After all a future change could
> cause completely unrelated errors, and you want those to be reported. If all
> you check is that there is an error, your test could be passing for the wrong
> reason. That's one bug hiding the other.
I've checked the strings that their Error() functions return.
>
> Apart from that though, good work. It's a shame the main function needs to
> loop API calls across all services and deployments, but I don't think there's
> anything to be done about it. Approved.
Thanks for the review!
Julian Edwards (julian-edwards) wrote : | # |
On 22/06/13 11:43, Jeroen T. Vermeulen wrote:
> Apart from that though, good work. It's a shame the main function needs to loop API calls across all services and deployments, but I don't think there's anything to be done about it. Approved.
I noted that too. The API doesn't seem exactly scalable here.
Raphaƫl Badin (rvb) wrote : | # |
Two small things (that could be done in follow-up branches if you agree with me that they should be done ;)):
- It would be good to exercise this new method in example/
- The provider operates within the confines of one hosted service so we need a method to list all the instances *in a specific hosted service*. We want to use structs to pass parameter to API methods at some point but I'd still recommend modifying the existing ListInstances() so that is takes a string parameter named 'hostedService' for now (i.e. like all the other methods in the management API). When we switch to using structs for parameters, we will change all the methods at once instead of having to live with a hybrid library.
I'm thinking about something like this: http://
Preview Diff
1 | === added file 'management.go' |
2 | --- management.go 1970-01-01 00:00:00 +0000 |
3 | +++ management.go 2013-06-22 14:10:33 +0000 |
4 | @@ -0,0 +1,24 @@ |
5 | +// Copyright 2013 Canonical Ltd. This software is licensed under the |
6 | +// GNU Lesser General Public License version 3 (see the file COPYING). |
7 | + |
8 | +package gwacl |
9 | + |
10 | +// ListInstances returns a slice of all instances for all deployments for all |
11 | +// services reachable with the user's credentials. |
12 | +func (api *ManagementAPI) ListInstances() ([]RoleInstance, error) { |
13 | + services, err := api.ListHostedServices() |
14 | + if err != nil { |
15 | + return nil, err |
16 | + } |
17 | + instances := []RoleInstance{} |
18 | + for _, service := range services { |
19 | + properties, err := api.GetHostedServiceProperties(service.ServiceName, true) |
20 | + if err != nil { |
21 | + return nil, err |
22 | + } |
23 | + for _, deployment := range properties.Deployments { |
24 | + instances = append(instances, deployment.RoleInstanceList...) |
25 | + } |
26 | + } |
27 | + return instances, nil |
28 | +} |
29 | |
30 | === renamed file 'management.go' => 'management_base.go' |
31 | === renamed file 'management_test.go' => 'management_base_test.go' |
32 | --- management_test.go 2013-06-21 11:58:18 +0000 |
33 | +++ management_base_test.go 2013-06-22 14:10:33 +0000 |
34 | @@ -13,12 +13,12 @@ |
35 | "time" |
36 | ) |
37 | |
38 | -type managementAPISuite struct { |
39 | +type managementBaseAPISuite struct { |
40 | x509DispatcherFixture |
41 | oldPollerInterval time.Duration |
42 | } |
43 | |
44 | -var _ = Suite(&managementAPISuite{}) |
45 | +var _ = Suite(&managementBaseAPISuite{}) |
46 | |
47 | func makeX509ResponseWithOperationHeader(operationID string) *x509Response { |
48 | header := http.Header{} |
49 | @@ -39,7 +39,7 @@ |
50 | return api |
51 | } |
52 | |
53 | -func (suite *managementAPISuite) TestGetOperationIDExtractsHeader(c *C) { |
54 | +func (suite *managementBaseAPISuite) TestGetOperationIDExtractsHeader(c *C) { |
55 | operationID := "operationID" |
56 | response := makeX509ResponseWithOperationHeader(operationID) |
57 | |
58 | @@ -48,7 +48,7 @@ |
59 | c.Check(returnedOperationID, Equals, operationID) |
60 | } |
61 | |
62 | -func (suite *managementAPISuite) TestBlockUntilCompletedSucceedsOnSyncSuccess(c *C) { |
63 | +func (suite *managementBaseAPISuite) TestBlockUntilCompletedSucceedsOnSyncSuccess(c *C) { |
64 | // Set of expected error returns for success statuses. |
65 | // (They all map to nil. It makes it easier further down to report |
66 | // unexpected errors in a helpful way). |
67 | @@ -69,7 +69,7 @@ |
68 | c.Check(receivedErrors, DeepEquals, expectedErrors) |
69 | } |
70 | |
71 | -func (suite *managementAPISuite) TestBlockUntilCompletedReturnsHTTPErrorOnSyncFailure(c *C) { |
72 | +func (suite *managementBaseAPISuite) TestBlockUntilCompletedReturnsHTTPErrorOnSyncFailure(c *C) { |
73 | // Set of failure statuses, and whether they're supposed to return |
74 | // HTTPError. |
75 | // (They all map to true. It makes it easier further down to report |
76 | @@ -96,7 +96,7 @@ |
77 | c.Check(receivedErrors, DeepEquals, expectedErrors) |
78 | } |
79 | |
80 | -func (suite *managementAPISuite) TestBlockUntilCompletedPollsOnAsyncOperation(c *C) { |
81 | +func (suite *managementBaseAPISuite) TestBlockUntilCompletedPollsOnAsyncOperation(c *C) { |
82 | const operationID = "async-operation-id" |
83 | // We set the dispatcher up for failure, and prove that |
84 | // blockUntilCompleted() makes a polling request. |
85 | @@ -118,7 +118,7 @@ |
86 | c.Check(polledOperationID, Equals, operationID) |
87 | } |
88 | |
89 | -func (suite *managementAPISuite) TestBlockUntilCompletedErrorsIfAsyncOperationFails(c *C) { |
90 | +func (suite *managementBaseAPISuite) TestBlockUntilCompletedErrorsIfAsyncOperationFails(c *C) { |
91 | response := DispatcherResponse{ |
92 | response: &x509Response{ |
93 | Body: []byte(fmt.Sprintf(operationXMLTemplate, "Failed")), |
94 | @@ -136,7 +136,7 @@ |
95 | c.Check(err, ErrorMatches, ".*asynchronous operation failed.*") |
96 | } |
97 | |
98 | -func (suite *managementAPISuite) TestBlockUntilCompletedErrorsOnInvalidXML(c *C) { |
99 | +func (suite *managementBaseAPISuite) TestBlockUntilCompletedErrorsOnInvalidXML(c *C) { |
100 | response := DispatcherResponse{ |
101 | response: &x509Response{ |
102 | Body: []byte(">invalidXML<"), |
103 | @@ -154,7 +154,7 @@ |
104 | c.Check(err, FitsTypeOf, new(xml.SyntaxError)) |
105 | } |
106 | |
107 | -func (suite *managementAPISuite) TestBlockUntilCompletedErrorsIfPollingFails(c *C) { |
108 | +func (suite *managementBaseAPISuite) TestBlockUntilCompletedErrorsIfPollingFails(c *C) { |
109 | response := DispatcherResponse{ |
110 | response: &x509Response{}, |
111 | errorObject: fmt.Errorf("unexpected error")} |
112 | @@ -169,7 +169,7 @@ |
113 | c.Check(err, ErrorMatches, ".*unexpected error.*") |
114 | } |
115 | |
116 | -func (suite *managementAPISuite) TestBlockUntilCompletedErrorsCanTimeout(c *C) { |
117 | +func (suite *managementBaseAPISuite) TestBlockUntilCompletedErrorsCanTimeout(c *C) { |
118 | response := &x509Response{ |
119 | Body: []byte(fmt.Sprintf(operationXMLTemplate, InProgressOperationStatus)), |
120 | StatusCode: http.StatusOK, |
121 | @@ -185,7 +185,7 @@ |
122 | c.Check(err, ErrorMatches, ".*polling timed out.*") |
123 | } |
124 | |
125 | -func (suite *managementAPISuite) TestBlockUntilCompletedSucceedsIfAsyncOperationSucceeds(c *C) { |
126 | +func (suite *managementBaseAPISuite) TestBlockUntilCompletedSucceedsIfAsyncOperationSucceeds(c *C) { |
127 | response := DispatcherResponse{ |
128 | response: &x509Response{ |
129 | Body: []byte(fmt.Sprintf(operationXMLTemplate, "Succeeded")), |
130 | @@ -203,7 +203,7 @@ |
131 | c.Assert(err, IsNil) |
132 | } |
133 | |
134 | -func (suite *managementAPISuite) TestNewManagementAPI(c *C) { |
135 | +func (suite *managementBaseAPISuite) TestNewManagementAPI(c *C) { |
136 | subscriptionId := "subscriptionId" |
137 | certFile := "certFile" |
138 | |
139 | @@ -215,14 +215,14 @@ |
140 | c.Assert(api.session.certFile, DeepEquals, session.certFile) |
141 | } |
142 | |
143 | -func (suite *managementAPISuite) TestNewManagementAPISetsDefaultPollerInterval(c *C) { |
144 | +func (suite *managementBaseAPISuite) TestNewManagementAPISetsDefaultPollerInterval(c *C) { |
145 | api, err := NewManagementAPI("subscriptionId", "certFile") |
146 | c.Assert(err, IsNil) |
147 | |
148 | c.Assert(api.PollerInterval, Equals, DefaultPollerInterval) |
149 | } |
150 | |
151 | -func (suite *managementAPISuite) TestNewManagementAPISetsDefaultPollerTimeout(c *C) { |
152 | +func (suite *managementBaseAPISuite) TestNewManagementAPISetsDefaultPollerTimeout(c *C) { |
153 | api, err := NewManagementAPI("subscriptionId", "certFile") |
154 | c.Assert(err, IsNil) |
155 | |
156 | @@ -232,7 +232,7 @@ |
157 | // setUpDispatcher sets up a request dispatcher that: |
158 | // - records requests |
159 | // - returns empty responses |
160 | -func (suite *managementAPISuite) setUpDispatcher() *[]*x509Request { |
161 | +func (suite *managementBaseAPISuite) setUpDispatcher() *[]*x509Request { |
162 | fixedResponse := x509Response{ |
163 | StatusCode: http.StatusOK, |
164 | Body: []byte{}, |
165 | @@ -258,7 +258,7 @@ |
166 | c.Assert(request.Method, Equals, Method) |
167 | } |
168 | |
169 | -func (suite *managementAPISuite) TestGetOperationFailsIfNoHeader(c *C) { |
170 | +func (suite *managementBaseAPISuite) TestGetOperationFailsIfNoHeader(c *C) { |
171 | response := x509Response{ |
172 | StatusCode: http.StatusOK, |
173 | } |
174 | @@ -267,7 +267,7 @@ |
175 | c.Check(err, ErrorMatches, ".*no operation header.*") |
176 | } |
177 | |
178 | -func (suite *managementAPISuite) TestListOSImagesRequestsListing(c *C) { |
179 | +func (suite *managementBaseAPISuite) TestListOSImagesRequestsListing(c *C) { |
180 | api := makeAPI(c) |
181 | rigFixedResponseDispatcher(&x509Response{StatusCode: http.StatusOK, Body: []byte("<Images></Images>")}) |
182 | requests := make([]*x509Request, 0) |
183 | @@ -280,7 +280,7 @@ |
184 | c.Check(requests[0].URL, Equals, api.session.composeURL("services/images")) |
185 | } |
186 | |
187 | -func (suite *managementAPISuite) TestListOSImagesReturnsImages(c *C) { |
188 | +func (suite *managementBaseAPISuite) TestListOSImagesReturnsImages(c *C) { |
189 | expectedImage := OSImage{ |
190 | LogicalSizeInGB: 199.0, |
191 | Label: MakeRandomString(10), |
192 | @@ -322,7 +322,7 @@ |
193 | c.Check(listing.Images[0], DeepEquals, expectedImage) |
194 | } |
195 | |
196 | -func (suite *managementAPISuite) TestListOSImagesPreservesOrdering(c *C) { |
197 | +func (suite *managementBaseAPISuite) TestListOSImagesPreservesOrdering(c *C) { |
198 | imageNames := []string{ |
199 | MakeRandomString(5), |
200 | MakeRandomString(5), |
201 | @@ -350,7 +350,7 @@ |
202 | c.Check(receivedNames, DeepEquals, imageNames) |
203 | } |
204 | |
205 | -func (suite *managementAPISuite) TestListHostedServices(c *C) { |
206 | +func (suite *managementBaseAPISuite) TestListHostedServices(c *C) { |
207 | api := makeAPI(c) |
208 | fixedResponse := x509Response{ |
209 | StatusCode: http.StatusOK, |
210 | @@ -369,7 +369,7 @@ |
211 | |
212 | // TODO test that ListHostedServices parses the structures correctly |
213 | |
214 | -func (suite *managementAPISuite) TestGetHostedServiceProperties_withoutDetails(c *C) { |
215 | +func (suite *managementBaseAPISuite) TestGetHostedServiceProperties_withoutDetails(c *C) { |
216 | api := makeAPI(c) |
217 | body := ` |
218 | <?xml version="1.0" encoding="utf-8"?> |
219 | @@ -416,7 +416,7 @@ |
220 | c.Check(len(properties.Deployments), Equals, 0) |
221 | } |
222 | |
223 | -func (suite *managementAPISuite) TestGetHostedServiceProperties_withDetails(c *C) { |
224 | +func (suite *managementBaseAPISuite) TestGetHostedServiceProperties_withDetails(c *C) { |
225 | api := makeAPI(c) |
226 | body := ` |
227 | <?xml version="1.0" encoding="utf-8"?> |
228 | @@ -468,7 +468,7 @@ |
229 | c.Check(properties.Deployments[0].Name, Equals, "name-of-deployment") |
230 | } |
231 | |
232 | -func (suite *managementAPISuite) TestAddHostedService(c *C) { |
233 | +func (suite *managementBaseAPISuite) TestAddHostedService(c *C) { |
234 | api := makeAPI(c) |
235 | recordedRequests := setUpDispatcher("operationID") |
236 | createHostedService := NewCreateHostedServiceWithLocation("testName", "testLabel", "East US") |
237 | @@ -480,7 +480,7 @@ |
238 | checkOneRequest(c, recordedRequests, expectedURL, expectedPayload, "POST") |
239 | } |
240 | |
241 | -func (suite *managementAPISuite) TestDeleteHostedService(c *C) { |
242 | +func (suite *managementBaseAPISuite) TestDeleteHostedService(c *C) { |
243 | api := makeAPI(c) |
244 | recordedRequests := setUpDispatcher("operationID") |
245 | hostedServiceName := "testName" |
246 | @@ -491,7 +491,7 @@ |
247 | checkOneRequest(c, recordedRequests, expectedURL, []byte{}, "DELETE") |
248 | } |
249 | |
250 | -func (suite *managementAPISuite) TestAddDeployment(c *C) { |
251 | +func (suite *managementBaseAPISuite) TestAddDeployment(c *C) { |
252 | api := makeAPI(c) |
253 | recordedRequests := setUpDispatcher("operationID") |
254 | serviceName := "serviceName" |
255 | @@ -508,7 +508,7 @@ |
256 | checkOneRequest(c, recordedRequests, expectedURL, expectedPayload, "POST") |
257 | } |
258 | |
259 | -func (suite *managementAPISuite) TestDeleteDeployment(c *C) { |
260 | +func (suite *managementBaseAPISuite) TestDeleteDeployment(c *C) { |
261 | api := makeAPI(c) |
262 | recordedRequests := setUpDispatcher("operationID") |
263 | hostedServiceName := "testHosterServiceName" |
264 | @@ -520,7 +520,7 @@ |
265 | checkOneRequest(c, recordedRequests, expectedURL, []byte{}, "DELETE") |
266 | } |
267 | |
268 | -func (suite *managementAPISuite) TestAddStorageAccount(c *C) { |
269 | +func (suite *managementBaseAPISuite) TestAddStorageAccount(c *C) { |
270 | api := makeAPI(c) |
271 | header := http.Header{} |
272 | header.Set("X-Ms-Request-Id", "operationID") |
273 | @@ -542,7 +542,7 @@ |
274 | checkOneRequest(c, &recordedRequests, expectedURL, expectedPayload, "POST") |
275 | } |
276 | |
277 | -func (suite *managementAPISuite) TestDeleteStorageAccount(c *C) { |
278 | +func (suite *managementBaseAPISuite) TestDeleteStorageAccount(c *C) { |
279 | const accountName = "myaccount" |
280 | api := makeAPI(c) |
281 | accountURL := api.session.composeURL("services/storageservices/" + accountName) |
282 | @@ -554,7 +554,7 @@ |
283 | checkOneRequest(c, recordedRequests, accountURL, nil, "DELETE") |
284 | } |
285 | |
286 | -func (suite *managementAPISuite) TestGetStorageAccountKeys(c *C) { |
287 | +func (suite *managementBaseAPISuite) TestGetStorageAccountKeys(c *C) { |
288 | const ( |
289 | accountName = "accountname" |
290 | primaryKey = "primarykey" |
291 | @@ -584,7 +584,7 @@ |
292 | c.Check(keys.URL, Equals, url) |
293 | } |
294 | |
295 | -func (suite *managementAPISuite) TestDeleteDisk(c *C) { |
296 | +func (suite *managementBaseAPISuite) TestDeleteDisk(c *C) { |
297 | // The current implementation of DeleteDisk() works around a bug in |
298 | // Windows Azure by polling the server. See the documentation in the file |
299 | // deletedisk.go for details. |
300 | @@ -600,7 +600,7 @@ |
301 | checkOneRequest(c, recordedRequests, expectedURL, []byte{}, "DELETE") |
302 | } |
303 | |
304 | -func (suite *managementAPISuite) TestPerformNodeOperation(c *C) { |
305 | +func (suite *managementBaseAPISuite) TestPerformNodeOperation(c *C) { |
306 | api := makeAPI(c) |
307 | recordedRequests := setUpDispatcher("operationID") |
308 | serviceName := "serviceName" |
309 | |
310 | === added file 'management_test.go' |
311 | --- management_test.go 1970-01-01 00:00:00 +0000 |
312 | +++ management_test.go 2013-06-22 14:10:33 +0000 |
313 | @@ -0,0 +1,180 @@ |
314 | +// Copyright 2013 Canonical Ltd. This software is licensed under the |
315 | +// GNU Lesser General Public License version 3 (see the file COPYING). |
316 | + |
317 | +package gwacl |
318 | + |
319 | +import ( |
320 | + . "launchpad.net/gocheck" |
321 | + "net/http" |
322 | +) |
323 | + |
324 | +type managementAPISuite struct{} |
325 | + |
326 | +var _ = Suite(&managementAPISuite{}) |
327 | + |
328 | +// TestListInstances goes through the happy path for ListInstances. |
329 | +func (suite *managementAPISuite) TestListInstances(c *C) { |
330 | + // First response is the list of services. |
331 | + services := HostedServiceDescriptorList{ |
332 | + HostedServices: []HostedServiceDescriptor{ |
333 | + {ServiceName: "S1"}, |
334 | + {ServiceName: "S2"}, |
335 | + }, |
336 | + } |
337 | + servicesXML, err := services.Serialize() |
338 | + c.Assert(err, IsNil) |
339 | + servicesResponse := DispatcherResponse{ |
340 | + response: &x509Response{ |
341 | + Body: []byte(servicesXML), |
342 | + StatusCode: http.StatusOK, |
343 | + }, |
344 | + } |
345 | + // Second response is the list of properties for "S1". |
346 | + propertiesS1 := HostedService{ |
347 | + ServiceName: "S1", |
348 | + Deployments: []Deployment{ |
349 | + { |
350 | + RoleInstanceList: []RoleInstance{ |
351 | + {RoleName: "one"}, |
352 | + {RoleName: "two"}, |
353 | + }, |
354 | + }, |
355 | + { |
356 | + RoleInstanceList: []RoleInstance{ |
357 | + {RoleName: "three"}, |
358 | + {RoleName: "four"}, |
359 | + }, |
360 | + }, |
361 | + }, |
362 | + } |
363 | + propertiesS1XML, err := propertiesS1.Serialize() |
364 | + c.Assert(err, IsNil) |
365 | + propertiesS1Response := DispatcherResponse{ |
366 | + response: &x509Response{ |
367 | + Body: []byte(propertiesS1XML), |
368 | + StatusCode: http.StatusOK, |
369 | + }, |
370 | + } |
371 | + // Third response is the list of properties for "S2". |
372 | + propertiesS2 := HostedService{ |
373 | + ServiceName: "S2", |
374 | + Deployments: []Deployment{ |
375 | + { |
376 | + RoleInstanceList: []RoleInstance{ |
377 | + {RoleName: "five"}, |
378 | + {RoleName: "six"}, |
379 | + }, |
380 | + }, |
381 | + { |
382 | + RoleInstanceList: []RoleInstance{ |
383 | + {RoleName: "seven"}, |
384 | + {RoleName: "eight"}, |
385 | + }, |
386 | + }, |
387 | + }, |
388 | + } |
389 | + propertiesS2XML, err := propertiesS2.Serialize() |
390 | + c.Assert(err, IsNil) |
391 | + propertiesS2Response := DispatcherResponse{ |
392 | + response: &x509Response{ |
393 | + Body: []byte(propertiesS2XML), |
394 | + StatusCode: http.StatusOK, |
395 | + }, |
396 | + } |
397 | + // Rig the prepared responses in. |
398 | + record := []*x509Request{} |
399 | + rigRecordingPreparedResponseDispatcher( |
400 | + &record, []DispatcherResponse{ |
401 | + servicesResponse, |
402 | + propertiesS1Response, |
403 | + propertiesS2Response, |
404 | + }, |
405 | + ) |
406 | + // Finally, exercise ListInstances. |
407 | + api := makeAPI(c) |
408 | + instances, err := api.ListInstances() |
409 | + c.Assert(err, IsNil) |
410 | + c.Check(instances, DeepEquals, []RoleInstance{ |
411 | + propertiesS1.Deployments[0].RoleInstanceList[0], |
412 | + propertiesS1.Deployments[0].RoleInstanceList[1], |
413 | + propertiesS1.Deployments[1].RoleInstanceList[0], |
414 | + propertiesS1.Deployments[1].RoleInstanceList[1], |
415 | + propertiesS2.Deployments[0].RoleInstanceList[0], |
416 | + propertiesS2.Deployments[0].RoleInstanceList[1], |
417 | + propertiesS2.Deployments[1].RoleInstanceList[0], |
418 | + propertiesS2.Deployments[1].RoleInstanceList[1], |
419 | + }) |
420 | + // The first request was for the services. |
421 | + c.Assert(len(record), Not(Equals), 0) |
422 | + c.Check(record[0], DeepEquals, &x509Request{ |
423 | + APIVersion: baseAPIVersion, |
424 | + URL: AZURE_URL + "subscriptionId/services/hostedservices", |
425 | + Method: "GET", |
426 | + }) |
427 | + // The second for S1's properties |
428 | + c.Assert(len(record), Not(Equals), 1) |
429 | + c.Check(record[1], DeepEquals, &x509Request{ |
430 | + APIVersion: baseAPIVersion, |
431 | + URL: AZURE_URL + "subscriptionId/services/hostedservices/S1?embed-detail=true", |
432 | + Method: "GET", |
433 | + }) |
434 | + // The third for S2's properties |
435 | + c.Assert(len(record), Not(Equals), 2) |
436 | + c.Check(record[2], DeepEquals, &x509Request{ |
437 | + APIVersion: baseAPIVersion, |
438 | + URL: AZURE_URL + "subscriptionId/services/hostedservices/S2?embed-detail=true", |
439 | + Method: "GET", |
440 | + }) |
441 | +} |
442 | + |
443 | +func (suite *managementAPISuite) TestListInstancesFailsGettingServices(c *C) { |
444 | + rigPreparedResponseDispatcher( |
445 | + []DispatcherResponse{ |
446 | + { |
447 | + response: &x509Response{ |
448 | + StatusCode: http.StatusUnauthorized, |
449 | + }, |
450 | + }, |
451 | + }, |
452 | + ) |
453 | + api := makeAPI(c) |
454 | + instances, err := api.ListInstances() |
455 | + c.Check(instances, IsNil) |
456 | + c.Assert(err, NotNil) |
457 | + c.Assert(err.Error(), Equals, "GET request failed (401: Unauthorized)") |
458 | +} |
459 | + |
460 | +func (suite *managementAPISuite) TestListInstancesFailsGettingDetails(c *C) { |
461 | + // First response is the list of services. |
462 | + services := HostedServiceDescriptorList{ |
463 | + HostedServices: []HostedServiceDescriptor{ |
464 | + {ServiceName: "S1"}, |
465 | + }, |
466 | + } |
467 | + servicesXML, err := services.Serialize() |
468 | + c.Assert(err, IsNil) |
469 | + servicesResponse := DispatcherResponse{ |
470 | + response: &x509Response{ |
471 | + Body: []byte(servicesXML), |
472 | + StatusCode: http.StatusOK, |
473 | + }, |
474 | + } |
475 | + // Second response to the properties request is Not Found, to simulate a |
476 | + // race perhaps. |
477 | + propertiesS1Response := DispatcherResponse{ |
478 | + response: &x509Response{ |
479 | + StatusCode: http.StatusNotFound, |
480 | + }, |
481 | + } |
482 | + rigPreparedResponseDispatcher( |
483 | + []DispatcherResponse{ |
484 | + servicesResponse, |
485 | + propertiesS1Response, |
486 | + }, |
487 | + ) |
488 | + api := makeAPI(c) |
489 | + instances, err := api.ListInstances() |
490 | + c.Check(instances, IsNil) |
491 | + c.Assert(err, NotNil) |
492 | + c.Assert(err.Error(), Equals, "GET request failed (404: Not Found)") |
493 | +} |
494 | |
495 | === modified file 'testhelpers_x509dispatch.go' |
496 | --- testhelpers_x509dispatch.go 2013-04-24 10:08:54 +0000 |
497 | +++ testhelpers_x509dispatch.go 2013-06-22 14:10:33 +0000 |
498 | @@ -56,6 +56,19 @@ |
499 | } |
500 | } |
501 | |
502 | +// rigRecordingPreparedResponseDispatcher sets up a request dispatcher that |
503 | +// returns, for each consecutive request, the next of a series of prepared |
504 | +// responses, and records each request. |
505 | +func rigRecordingPreparedResponseDispatcher(record *[]*x509Request, responses []DispatcherResponse) { |
506 | + index := 0 |
507 | + _X509Dispatcher = func(session *x509Session, request *x509Request) (*x509Response, error) { |
508 | + *record = append(*record, request) |
509 | + response := responses[index] |
510 | + index += 1 |
511 | + return response.response, response.errorObject |
512 | + } |
513 | +} |
514 | + |
515 | // setUpDispatcher sets up a request dispatcher that: |
516 | // - records requests |
517 | // - returns empty responses |
518 | |
519 | === modified file 'xmlobjects.go' |
520 | --- xmlobjects.go 2013-06-21 12:24:53 +0000 |
521 | +++ xmlobjects.go 2013-06-22 14:10:33 +0000 |
522 | @@ -296,10 +296,18 @@ |
523 | Deployments []Deployment `xml:"Deployments>Deployment"` |
524 | } |
525 | |
526 | +func (c HostedService) Serialize() (string, error) { |
527 | + return toxml(c) |
528 | +} |
529 | + |
530 | type HostedServiceDescriptorList struct { |
531 | HostedServices []HostedServiceDescriptor `xml:"HostedService"` |
532 | } |
533 | |
534 | +func (c *HostedServiceDescriptorList) Serialize() (string, error) { |
535 | + return toxml(c) |
536 | +} |
537 | + |
538 | // HostedServiceDescriptor contains a subset of the details in HostedService, |
539 | // and is used when describing a list of HostedServices. |
540 | type HostedServiceDescriptor struct { |
It's depressing that it's all so much work, that the code gets so verbose. And that makes me hesitant to suggest further improvements that involve yet more work. Nevertheless, here's what I came up with:
1. The way rigRecordingPre paredResponseDi spatcher pops items off "responses" was not entirely obvious to me, what with the combination of slices, closures, and the returned pointer to a nested function. A comment might help.
2. There's a NotNil checker, shorthand for Not(IsNil).
3. It'd be good to test at least *something* about the errors you get back, not just the fact that you get an error. After all a future change could cause completely unrelated errors, and you want those to be reported. If all you check is that there is an error, your test could be passing for the wrong reason. That's one bug hiding the other.
Apart from that though, good work. It's a shame the main function needs to loop API calls across all services and deployments, but I don't think there's anything to be done about it. Approved.