Merge lp:~jtv/gwacl/block-on-accepted into lp:gwacl

[Jeroen T. Vermeulen]

Block on additional API operations that *might* require polling.

You'll notice that I did not test for this. Believe me, it took a lot of soul-searching. Am I not just making up excuses?

But the cost-benefit ratio is far out of whack. It's just a trivial function call (so in Go, 4 lines of code with a cyclomatic complexity of 2, sigh) and it happens in many many places. Most of this branch would be brain-dead repetitive testing. And the main risk that a test suite should guard us against is for is forgetting to make the call in the first place. If we can forget to add the call in one function, chances are we'll forget the test for that function as well!

Instead, I documented the need to call blockUntilCompleted() on the post(), put(), and delete() methods.

Jeroen

[Raphaël Badin (rvb)]

Looks good, but I confess I'm not entirely happy with the lack of tests. Looks like waiting for the operation to complete is a pretty important piece of what this lib does now.

> And the main risk that a test suite should guard us against is for is forgetting to make the call in the first place.
> If we can forget to add the call in one function, chances are we'll forget the test for that function as well!

The tests for the management methods are pretty repetitive and we often copy and paste an existing test when creating a new test for a new method. For this reason, I don't think we are at risk of forgetting to test that the call to blockUntilCompleted() is there because it will already be in the copied test.

Also, you talk about adding new methods but the tests also guard us against non-intended changes to the existing code.

How about something along the lines of: http://paste.ubuntu.com/5670027/

review: Approve

[Jeroen T. Vermeulen (jtv)]

It's a cool trick, but it's also sneaking yet another aspect into the existing tests, increasing duplication of coverage and reducing exactly the sort of flexibility you were talking about the other day: what will we have to change if Azure works slightly differently than we've been led to believe? And no matter how we look at it, we still have to cover a very wide API surface without omissions. I did consider regressions, but I can't see them as the biggest risk and I am wary of diminishing returns.

So here's another idea: a thin layer on top of get()/post()/delete()/put(), inside ManagementAPI. For get() it can have the deserialization built in, and for post()/delete()/put() it includes the polling. We just make the right way to do it easier than the wrong way, and eliminate some annoying duplication to boot.

[Jeroen T. Vermeulen (jtv)]

(Better land this in the meantime, because the clock is ticking and we can resolve this in a fresh branch)

=== modified file 'managementapi.go'
--- managementapi.go	2013-04-02 08:51:18 +0000
+++ managementapi.go	2013-04-02 09:18:24 +0000
@@ -135,8 +135,11 @@
 }
 
 func (api *ManagementAPI) DeleteHostedService(serviceName string) error {
-    _, err := api.session.delete("services/hostedservices/" + serviceName)
-    return err
+    response, err := api.session.delete("services/hostedservices/" + serviceName)
+    if err != nil {
+        return err
+    }
+    return api.blockUntilCompleted(response)
 }
 
 // AddDeployment adds a virtual machine deployment.
@@ -156,8 +159,11 @@
 
 func (api *ManagementAPI) DeleteDeployment(serviceName string, deploymentName string) error {
     path := "services/hostedservices/" + serviceName + "/deployments/" + deploymentName
-    _, err := api.session.delete(path)
-    return err
+    response, err := api.session.delete(path)
+    if err != nil {
+        return err
+    }
+    return api.blockUntilCompleted(response)
 }
 
 // AddStorageAccount starts the creation of a storage account.  This is
@@ -179,8 +185,11 @@
 
 // DeleteStorageAccount deletes a storage account.
 func (api *ManagementAPI) DeleteStorageAccount(storageAccountName string) error {
-    _, err := api.session.delete("services/storageservices/" + storageAccountName)
-    return err
+    response, err := api.session.delete("services/storageservices/" + storageAccountName)
+    if err != nil {
+        return err
+    }
+    return api.blockUntilCompleted(response)
 }
 
 // GetStorageAccountKeys retrieves a storage account's primary and secondary
@@ -200,8 +209,11 @@
 }
 
 func (api *ManagementAPI) DeleteDisk(diskName string) error {
-    _, err := api.session.delete("services/disks/" + diskName)
-    return err
+    response, err := api.session.delete("services/disks/" + diskName)
+    if err != nil {
+        return err
+    }
+    return api.blockUntilCompleted(response)
 }
 
 // Perform an operation on the specified role (as defined by serviceName, deploymentName and

=== modified file 'x509session.go'
--- x509session.go	2013-04-01 09:16:04 +0000
+++ x509session.go	2013-04-02 09:18:24 +0000
@@ -64,6 +64,8 @@
 // It returns the response body and/or an error.  If the error is a
 // ServerError, the returned body will be the one received from the server.
 // For any other kind of error, the returned body will be nil.
+// Be aware that Azure may perform POST operations asynchronously.  If you are
+// not sure, call blockUntilCompleted() on the response.
 func (session *x509Session) post(path string, body []byte, contentType string) (*x509Response, error) {
     request := newX509RequestPOST(session.composeURL(path), body, contentType)
     response, err := _X509Dispatcher(session, request)
@@ -75,6 +77,8 @@
 }
 
 // delete performs a DELETE request to the Azure management API.
+// Be aware that Azure may perform DELETE operations asynchronously.  If you
+// are not sure, call blockUntilCompleted() on the response.
 func (session *x509Session) delete(path string) (*x509Response, error) {
     request := newX509RequestDELETE(session.composeURL(path))
     response, err := _X509Dispatcher(session, request)
@@ -86,11 +90,14 @@
 }
 
 // put performs a PUT request to the Azure management API.
+// Be aware that Azure may perform PUT operations asynchronously.  If you are
+// not sure, call blockUntilCompleted() on the response.
 func (session *x509Session) put(path string, body []byte) (*x509Response, error) {
     request := newX509RequestPUT(session.composeURL(path), body)
     response, err := _X509Dispatcher(session, request)
     if err != nil {
         return nil, err
     }
-    return response, session.getServerError(response.StatusCode, response.Body, "POST request failed")
+    err = session.getServerError(response.StatusCode, response.Body, "POST request failed")
+    return response, err
 }