Merge lp:~julian-edwards/gwacl/delete-disk-media into lp:gwacl

[Julian Edwards]

Add a deleteBlob parameter to DeleteDisk, which tells Azure to delete the associated blob storage.

[lp:~julian-edwards/gwacl/delete-disk-media]

188. By Julian Edwards on 2013-07-16

format

[Raphaël Badin (rvb)]

Looks good!

[0]

92 // TODO: set comp=media to optionally delete associated blob.
93 -func (api *ManagementAPI) DeleteDisk(diskName string) error {
94 +func (api *ManagementAPI) DeleteDisk(diskName string, deleteBlob bool) error {

You can probably get rid of the TODO comment here (or better, turn it into a sentence explaining what 'deleteBlob' does).

[1]

92 // TODO: set comp=media to optionally delete associated blob.
93 -func (api *ManagementAPI) DeleteDisk(diskName string) error {
94 +func (api *ManagementAPI) DeleteDisk(diskName string, deleteBlob bool) error {

Since you're breaking backward compatibility already, what about putting all the parameters in a DeleteDiskRequest object?

review: Approve

[lp:~julian-edwards/gwacl/delete-disk-media]

189. By Julian Edwards on 2013-07-17

Make DeleteDisk take a request struct param

190. By Julian Edwards on 2013-07-17

FORMAT

[Julian Edwards (julian-edwards)]

Thanks for the review, and thanks for spotting that I should have changed to a request struct!

At some point, we need to bite the bullet and fix all the call sites before the juju provider is "released"

=== modified file 'deletedisk.go'
--- deletedisk.go	2013-04-17 08:10:24 +0000
+++ deletedisk.go	2013-07-17 01:01:23 +0000
@@ -27,14 +27,15 @@
 var deleteDiskInterval = 10 * time.Second
 
 type diskDeletePoller struct {
-    api      *ManagementAPI
-    diskName string
+    api        *ManagementAPI
+    diskName   string
+    deleteBlob bool
 }
 
 var _ poller = &diskDeletePoller{}
 
 func (poller diskDeletePoller) poll() (*x509Response, error) {
-    return nil, poller.api._DeleteDisk(poller.diskName)
+    return nil, poller.api._DeleteDisk(poller.diskName, poller.deleteBlob)
 }
 
 // isInUseError returns whether or not the given string is of the "disk in use"

=== modified file 'deletedisk_test.go'
--- deletedisk_test.go	2013-07-12 04:22:16 +0000
+++ deletedisk_test.go	2013-07-17 01:01:23 +0000
@@ -38,7 +38,7 @@
 }
 
 func (suite *deleteDiskSuite) TestIsDoneReturnsTrueIfNilError(c *C) {
-    poller := diskDeletePoller{nil, ""}
+    poller := diskDeletePoller{nil, "", false}
     randomResponse := x509Response{StatusCode: http.StatusAccepted}
     done, err := poller.isDone(&randomResponse, nil)
     c.Check(done, Equals, true)
@@ -48,7 +48,7 @@
 func (suite *deleteDiskSuite) TestIsDoneReturnsFalseIfDiskInUseError(c *C) {
     diskName := "gwacldiske5w7lkj"
     diskInUseError := fmt.Errorf(diskInUseErrorTemplate, diskName)
-    poller := diskDeletePoller{nil, diskName}
+    poller := diskDeletePoller{nil, diskName, false}
     done, err := poller.isDone(nil, diskInUseError)
     c.Check(done, Equals, false)
     c.Check(err, IsNil)
@@ -56,7 +56,7 @@
 
 func (suite *deleteDiskSuite) TestIsDoneReturnsTrueIfAnotherError(c *C) {
     anotherError := fmt.Errorf("Unknown error")
-    poller := diskDeletePoller{nil, "disk-name"}
+    poller := diskDeletePoller{nil, "disk-name", false}
     done, err := poller.isDone(nil, anotherError)
     c.Check(done, Equals, true)
     c.Check(err, Equals, anotherError)
@@ -66,7 +66,7 @@
     api := makeAPI(c)
     recordedRequests := setUpDispatcher("operationID")
     diskName := "gwacldiske5w7lkj"
-    poller := diskDeletePoller{api, diskName}
+    poller := diskDeletePoller{api, diskName, false}
 
     response, err := poller.poll()
 
@@ -90,7 +90,7 @@
 
     api := makeAPI(c)
     diskName := "gwacldiske5w7lkj"
-    poller := diskDeletePoller{api, diskName}
+    poller := diskDeletePoller{api, diskName, false}
 
     response, err := performPolling(poller, time.Nanosecond, time.Minute)
 

=== modified file 'management.go'
--- management.go	2013-07-12 12:55:23 +0000
+++ management.go	2013-07-17 01:01:23 +0000
@@ -158,7 +158,9 @@
     sort.Strings(diskNames)
     // 3. Delete the disks.
     for _, diskName := range diskNames {
-        err = api.DeleteDisk(diskName)
+        err = api.DeleteDisk(&DeleteDiskRequest{
+            DiskName:   diskName,
+            DeleteBlob: false}) // change to true at some point?
         if err != nil && !IsNotFoundError(err) {
             return err
         }

=== modified file 'management_base.go'
--- management_base.go	2013-07-16 15:43:48 +0000
+++ management_base.go	2013-07-17 01:01:23 +0000
@@ -290,20 +290,27 @@
     return &keys, nil
 }
 
+type DeleteDiskRequest struct {
+    DiskName   string // Name of the disk to delete.
+    DeleteBlob bool   // Whether to delete the associated blob storage.
+}
+
 // DeleteDisk deletes the named OS/data disk.
 // See http://msdn.microsoft.com/en-us/library/windowsazure/jj157200.aspx
-// TODO: set comp=media to optionally delete associated blob.
-func (api *ManagementAPI) DeleteDisk(diskName string) error {
+func (api *ManagementAPI) DeleteDisk(request *DeleteDiskRequest) error {
     // Use the disk deletion poller to work around a bug in Windows Azure.
     // See the documentation in the file deletedisk.go for details.
-    poller := diskDeletePoller{api, diskName}
+    poller := diskDeletePoller{api, request.DiskName, request.DeleteBlob}
     _, err := performPolling(poller, deleteDiskInterval, deleteDiskTimeout)
     return err
 }
 
-func (api *ManagementAPI) _DeleteDisk(diskName string) error {
-    response, err := api.session.delete(
-        "services/disks/"+diskName, "2012-08-01")
+func (api *ManagementAPI) _DeleteDisk(diskName string, deleteBlob bool) error {
+    url := "services/disks/" + diskName
+    if deleteBlob {
+        url = addURLQueryParams(url, "comp", "media")
+    }
+    response, err := api.session.delete(url, "2012-08-01")
     if err != nil {
         return err
     }

=== modified file 'management_base_test.go'
--- management_base_test.go	2013-07-16 15:43:48 +0000
+++ management_base_test.go	2013-07-17 01:01:23 +0000
@@ -10,6 +10,7 @@
     "fmt"
     . "launchpad.net/gocheck"
     "net/http"
+    "net/url"
     "strings"
     "time"
 )
@@ -754,13 +755,32 @@
     api := makeAPI(c)
     recordedRequests := setUpDispatcher("operationID")
     diskName := "diskName"
-    err := api.DeleteDisk(diskName)
+    err := api.DeleteDisk(&DeleteDiskRequest{DiskName: diskName})
 
     c.Assert(err, IsNil)
     c.Assert(*recordedRequests, HasLen, 1)
     assertDeleteDiskRequest(c, api, diskName, (*recordedRequests)[0])
 }
 
+func (suite *managementBaseAPISuite) TestDeleteDiskWithDeleteBlob(c *C) {
+    // Setting deleteBlob=true should append comp=media as a url query value.
+    deleteDiskInterval = time.Nanosecond
+    api := makeAPI(c)
+    recordedRequests := setUpDispatcher("operationID")
+    diskName := "diskName"
+
+    err := api.DeleteDisk(&DeleteDiskRequest{
+        DiskName: diskName, DeleteBlob: true})
+
+    c.Assert(err, IsNil)
+    originalURL := (*recordedRequests)[0].URL
+    parsedURL, err := url.Parse(originalURL)
+    c.Assert(err, IsNil)
+    values := parsedURL.Query()
+    c.Assert(err, IsNil)
+    c.Check(values["comp"], DeepEquals, []string{"media"})
+}
+
 func (suite *managementBaseAPISuite) TestPerformNodeOperation(c *C) {
     api := makeAPI(c)
     recordedRequests := setUpDispatcher("operationID")