Merge lp:~rvb/gwacl/api-polling-fix into lp:gwacl

[Raphaël Badin]

Fix api.blockUntilCompleted so that it returns an error when the asynchronous operation fails.

Drive-by fix: add tests for blockUntilCompleted.

[Jeroen T. Vermeulen (jtv)]

Excellent work. I suppose we'll want to improve the failure reporting later; we get some more helpful information from asynchronous operations IIRC.

This bit in managementapi_test.go looks a little weird:

80 +func makeAPI(c *C) *ManagementAPI {
81 + subscriptionId := "subscriptionId"
82 + subscriptionId = subscriptionId

What does line 82 do for us and why doesn't the compiler warn us about it?

review: Approve

[Raphaël Badin (rvb)]

> 80 +func makeAPI(c *C) *ManagementAPI {
> 81 + subscriptionId := "subscriptionId"
> 82 + subscriptionId = subscriptionId
>
> What does line 82 do for us and why doesn't the compiler warn us about it?

Oosp, a copy and paste error I'm guessing… I don't see what the compiler would have to say about it… apart from the fact that this line does nothing :).

[Julian Edwards (julian-edwards)]

Attempt to merge into lp:gwacl failed due to conflicts:

text conflict in managementapi_test.go

[lp:~rvb/gwacl/api-polling-fix]

77. By Raphaël Badin on 2013-03-29

Merge trunk.

[Jeroen T. Vermeulen (jtv)]

Yes, my point about the compiler was that the Go people made such a big thing about rooting out unused variables and such — but then they happily accept dead assignments, self-assignments, unused functions and classes, and ignored error values.

=== modified file 'managementapi.go'
--- managementapi.go	2013-03-29 12:22:47 +0000
+++ managementapi.go	2013-03-29 13:33:21 +0000
@@ -42,22 +42,25 @@
 var pollerInterval = 10 * time.Second
 
 // blockUntilCompleted blocks until the operation referenced in the headers of the given response
-// is completed.
+// is completed.  It returns an error if the given response does not contain
+// a reference to an asychronous operation, if the response from the server
+// cannot be parsed, or if the asynchronous operation fails.
 // Internally, it polls the server at regular intervals until the operation is completed.
 func (api *ManagementAPI) blockUntilCompleted(response *x509Response) error {
-    // TODO: blockUntilCompleted should return an error when the operation
-    // fails.  Right now it simply block until the operation fails or
-    // succeeds.
     if pollerInterval != 0 {
         operationID, err := getOperationID(response)
         if err != nil {
             return err
         }
         poller := newOperationPoller(api, operationID)
-        _, err = performOperationPolling(poller, pollerInterval)
+        operation, err := performOperationPolling(poller, pollerInterval)
         if err != nil {
             return err
         }
+        if operation.Status != SucceededOperationStatus {
+            err := fmt.Errorf("asychronous operation failed (status: %v)", operation.Status)
+            return err
+        }
     }
     return nil
 }

=== modified file 'managementapi_test.go'
--- managementapi_test.go	2013-03-29 12:26:23 +0000
+++ managementapi_test.go	2013-03-29 13:33:21 +0000
@@ -4,30 +4,123 @@
 package gwacl
 
 import (
+    "encoding/xml"
     "fmt"
     . "launchpad.net/gocheck"
     "net/http"
     "time"
 )
 
-type managementAPIUtilitiesSuite struct{}
+type managementAPIUtilitiesSuite struct {
+    oldPollerInterval time.Duration
+}
 
 var _ = Suite(&managementAPIUtilitiesSuite{})
 
+func (suite *managementAPIUtilitiesSuite) SetUpTest(c *C) {
+    // Record the original poller interval. Will be restored at the end of
+    // each test.
+    suite.oldPollerInterval = pollerInterval
+    // Set the poller interval to something short to speed up the tests.
+    pollerInterval = time.Millisecond
+}
+
+func (suite *managementAPIUtilitiesSuite) TearDownTest(c *C) {
+    // Restore old poller interval.
+    pollerInterval = suite.oldPollerInterval
+}
+
+func makeX509ResponseWithOperationHeader(operationID string) *x509Response {
+    header := http.Header{}
+    header.Set(operationIDHeaderName, operationID)
+    response := x509Response{
+        StatusCode: http.StatusAccepted,
+        Header:     header,
+    }
+    return &response
+}
+
+func makeAPI(c *C) *ManagementAPI {
+    subscriptionId := "subscriptionId"
+    api, err := NewManagementAPI(subscriptionId, "certFile")
+    c.Assert(err, IsNil)
+    return api
+}
+
 func (suite *managementAPIUtilitiesSuite) TestGetOperationIDExtractsHeader(c *C) {
-    header := http.Header{}
     operationID := "operationID"
-    header.Set(operationIDHeaderName, operationID)
-    response := x509Response{
-        StatusCode: http.StatusAccepted,
-        Header:     header,
-    }
+    response := makeX509ResponseWithOperationHeader(operationID)
 
-    returnedOperationID, err := getOperationID(&response)
+    returnedOperationID, err := getOperationID(response)
     c.Assert(err, IsNil)
     c.Check(returnedOperationID, Equals, operationID)
 }
 
+func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedErrorsIfOperationFails(c *C) {
+    response := DispatcherResponse{
+        response: &x509Response{
+            Body:       []byte(fmt.Sprintf(operationXMLTemplate, "Failed")),
+            StatusCode: http.StatusOK,
+        },
+        errorObject: nil}
+    responses := []DispatcherResponse{response}
+    rigPreparedResponseDispatcher(responses)
+    operationID := "operationID"
+    operationResponse := makeX509ResponseWithOperationHeader(operationID)
+    api := makeAPI(c)
+
+    err := api.blockUntilCompleted(operationResponse)
+    c.Check(err, ErrorMatches, ".*asychronous operation failed.*")
+}
+
+func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedErrorsIfInvalidXML(c *C) {
+    response := DispatcherResponse{
+        response: &x509Response{
+            Body:       []byte(">invalidXML<"),
+            StatusCode: http.StatusOK,
+        },
+        errorObject: nil}
+    responses := []DispatcherResponse{response}
+    rigPreparedResponseDispatcher(responses)
+    operationID := "operationID"
+    operationResponse := makeX509ResponseWithOperationHeader(operationID)
+    api := makeAPI(c)
+
+    err := api.blockUntilCompleted(operationResponse)
+    c.Check(err, FitsTypeOf, new(xml.SyntaxError))
+}
+
+func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedErrorsIfFailsToPoll(c *C) {
+    response := DispatcherResponse{
+        response:    &x509Response{},
+        errorObject: fmt.Errorf("unexpected error")}
+    responses := []DispatcherResponse{response}
+    rigPreparedResponseDispatcher(responses)
+    operationID := "operationID"
+    operationResponse := makeX509ResponseWithOperationHeader(operationID)
+    api := makeAPI(c)
+
+    err := api.blockUntilCompleted(operationResponse)
+    c.Check(err, ErrorMatches, ".*unexpected error.*")
+}
+
+func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedSucceedsIfOperationSucceeds(c *C) {
+    response := DispatcherResponse{
+        response: &x509Response{
+            Body:       []byte(fmt.Sprintf(operationXMLTemplate, "Succeeded")),
+            StatusCode: http.StatusOK,
+        },
+        errorObject: nil}
+    responses := []DispatcherResponse{response}
+    rigPreparedResponseDispatcher(responses)
+    operationID := "operationID"
+    operationResponse := makeX509ResponseWithOperationHeader(operationID)
+    api := makeAPI(c)
+
+    err := api.blockUntilCompleted(operationResponse)
+    c.Assert(err, IsNil)
+}
+
 type managementAPISuite struct {
     x509DispatcherFixture
     oldPollerInterval time.Duration
@@ -60,14 +153,6 @@
     c.Assert(api.session.certFile, DeepEquals, session.certFile)
 }
 
-func (suite *managementAPISuite) makeAPI(c *C) *ManagementAPI {
-    subscriptionId := "subscriptionId"
-    subscriptionId = subscriptionId
-    api, err := NewManagementAPI(subscriptionId, "certFile")
-    c.Assert(err, IsNil)
-    return api
-}
-
 // setUpDispatcher sets up a request dispatcher that:
 // - records requests
 // - returns empty responses
@@ -107,7 +192,7 @@
 }
 
 func (suite *managementAPISuite) TestListHostedServiceDescriptors(c *C) {
-    api := suite.makeAPI(c)
+    api := makeAPI(c)
     fixedResponse := x509Response{
         StatusCode: http.StatusOK,
         Body:       []byte("<HostedServices></HostedServices>"),
@@ -126,7 +211,7 @@
 // TODO test that ListHostedServiceDescriptors parses the structures correctly
 
 func (suite *managementAPISuite) TestLoadHostedService(c *C) {
-    api := suite.makeAPI(c)
+    api := makeAPI(c)
     fixedResponse := x509Response{
         StatusCode: http.StatusOK,
         Body:       []byte("<HostedService></HostedService>"),
@@ -147,7 +232,7 @@
 // TODO test that TestLoadHostedService parses the structures correctly
 
 func (suite *managementAPISuite) TestAddHostedService(c *C) {
-    api := suite.makeAPI(c)
+    api := makeAPI(c)
     recordedRequests := setUpDispatcher("operationID")
     createHostedService := NewCreateHostedServiceWithLocation("testName", "testLabel", "East US")
     err := api.AddHostedService(createHostedService)
@@ -159,7 +244,7 @@
 }
 
 func (suite *managementAPISuite) TestDeleteHostedService(c *C) {
-    api := suite.makeAPI(c)
+    api := makeAPI(c)
     recordedRequests := setUpDispatcher("operationID")
     hostedServiceName := "testName"
     err := api.DeleteHostedService(hostedServiceName)
@@ -170,7 +255,7 @@
 }
 
 func (suite *managementAPISuite) TestAddDeployment(c *C) {
-    api := suite.makeAPI(c)
+    api := makeAPI(c)
     recordedRequests := setUpDispatcher("operationID")
     serviceName := "serviceName"
     configurationSet := NewLinuxProvisioningConfiguration("testHostname12345", "test", "test123#@!", false)
@@ -187,7 +272,7 @@
 }
 
 func (suite *managementAPISuite) TestDeleteDeployment(c *C) {
-    api := suite.makeAPI(c)
+    api := makeAPI(c)
     recordedRequests := setUpDispatcher("operationID")
     hostedServiceName := "testHosterServiceName"
     deploymentName := "testDeploymentName"
@@ -199,7 +284,7 @@
 }
 
 func (suite *managementAPISuite) TestAddStorageAccount(c *C) {
-    api := suite.makeAPI(c)
+    api := makeAPI(c)
     header := http.Header{}
     header.Set("X-Ms-Request-Id", "operationID")
     fixedResponse := x509Response{
@@ -226,8 +311,8 @@
         primaryKey   = "primarykey"
         secondaryKey = "secondarykey"
     )
-    api := suite.makeAPI(c)
-    url := api.session.composeURL("services/storageservices/"+accountName)
+    api := makeAPI(c)
+    url := api.session.composeURL("services/storageservices/" + accountName)
     body := fmt.Sprintf(
         `<StorageService>
             <Url>%s</Url>
@@ -251,7 +336,7 @@
 }
 
 func (suite *managementAPISuite) TestPerformNodeOperation(c *C) {
-    api := suite.makeAPI(c)
+    api := makeAPI(c)
     recordedRequests := setUpDispatcher("operationID")
     serviceName := "serviceName"
     deploymentName := "deploymentName"

=== modified file 'xmlobjects.go'
--- xmlobjects.go	2013-03-27 17:08:58 +0000
+++ xmlobjects.go	2013-03-29 13:33:21 +0000
@@ -525,7 +525,10 @@
 // Operation Services bits
 //
 
-var InProgressOperationStatus = "InProgress"
+const (
+    InProgressOperationStatus = "InProgress"
+    SucceededOperationStatus  = "Succeeded"
+)
 
 type Operation struct {
     ID             string `xml:"ID"`