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

Proposed by Julian Edwards
Status: Merged
Approved by: Julian Edwards
Approved revision: 190
Merged at revision: 186
Proposed branch: lp:~julian-edwards/gwacl/delete-disk-media
Merge into: lp:gwacl
Diff against target: 170 lines (+46/-16)
5 files modified
deletedisk.go (+4/-3)
deletedisk_test.go (+5/-5)
management.go (+3/-1)
management_base.go (+13/-6)
management_base_test.go (+21/-1)
To merge this branch: bzr merge lp:~julian-edwards/gwacl/delete-disk-media
Reviewer Review Type Date Requested Status
Raphaël Badin (community) Approve
Review via email: mp+174926@code.launchpad.net

Commit message

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

To post a comment you must log in.
188. By Julian Edwards

format

Revision history for this message
Raphaël Badin (rvb) wrote :

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
189. By Julian Edwards

Make DeleteDisk take a request struct param

190. By Julian Edwards

FORMAT

Revision history for this message
Julian Edwards (julian-edwards) wrote :

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"

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
27var deleteDiskInterval = 10 * time.Second27var deleteDiskInterval = 10 * time.Second
2828
29type diskDeletePoller struct {29type diskDeletePoller struct {
30 api *ManagementAPI30 api *ManagementAPI
31 diskName string31 diskName string
32 deleteBlob bool
32}33}
3334
34var _ poller = &diskDeletePoller{}35var _ poller = &diskDeletePoller{}
3536
36func (poller diskDeletePoller) poll() (*x509Response, error) {37func (poller diskDeletePoller) poll() (*x509Response, error) {
37 return nil, poller.api._DeleteDisk(poller.diskName)38 return nil, poller.api._DeleteDisk(poller.diskName, poller.deleteBlob)
38}39}
3940
40// isInUseError returns whether or not the given string is of the "disk in use"41// isInUseError returns whether or not the given string is of the "disk in use"
4142
=== 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 @@
38}38}
3939
40func (suite *deleteDiskSuite) TestIsDoneReturnsTrueIfNilError(c *C) {40func (suite *deleteDiskSuite) TestIsDoneReturnsTrueIfNilError(c *C) {
41 poller := diskDeletePoller{nil, ""}41 poller := diskDeletePoller{nil, "", false}
42 randomResponse := x509Response{StatusCode: http.StatusAccepted}42 randomResponse := x509Response{StatusCode: http.StatusAccepted}
43 done, err := poller.isDone(&randomResponse, nil)43 done, err := poller.isDone(&randomResponse, nil)
44 c.Check(done, Equals, true)44 c.Check(done, Equals, true)
@@ -48,7 +48,7 @@
48func (suite *deleteDiskSuite) TestIsDoneReturnsFalseIfDiskInUseError(c *C) {48func (suite *deleteDiskSuite) TestIsDoneReturnsFalseIfDiskInUseError(c *C) {
49 diskName := "gwacldiske5w7lkj"49 diskName := "gwacldiske5w7lkj"
50 diskInUseError := fmt.Errorf(diskInUseErrorTemplate, diskName)50 diskInUseError := fmt.Errorf(diskInUseErrorTemplate, diskName)
51 poller := diskDeletePoller{nil, diskName}51 poller := diskDeletePoller{nil, diskName, false}
52 done, err := poller.isDone(nil, diskInUseError)52 done, err := poller.isDone(nil, diskInUseError)
53 c.Check(done, Equals, false)53 c.Check(done, Equals, false)
54 c.Check(err, IsNil)54 c.Check(err, IsNil)
@@ -56,7 +56,7 @@
5656
57func (suite *deleteDiskSuite) TestIsDoneReturnsTrueIfAnotherError(c *C) {57func (suite *deleteDiskSuite) TestIsDoneReturnsTrueIfAnotherError(c *C) {
58 anotherError := fmt.Errorf("Unknown error")58 anotherError := fmt.Errorf("Unknown error")
59 poller := diskDeletePoller{nil, "disk-name"}59 poller := diskDeletePoller{nil, "disk-name", false}
60 done, err := poller.isDone(nil, anotherError)60 done, err := poller.isDone(nil, anotherError)
61 c.Check(done, Equals, true)61 c.Check(done, Equals, true)
62 c.Check(err, Equals, anotherError)62 c.Check(err, Equals, anotherError)
@@ -66,7 +66,7 @@
66 api := makeAPI(c)66 api := makeAPI(c)
67 recordedRequests := setUpDispatcher("operationID")67 recordedRequests := setUpDispatcher("operationID")
68 diskName := "gwacldiske5w7lkj"68 diskName := "gwacldiske5w7lkj"
69 poller := diskDeletePoller{api, diskName}69 poller := diskDeletePoller{api, diskName, false}
7070
71 response, err := poller.poll()71 response, err := poller.poll()
7272
@@ -90,7 +90,7 @@
9090
91 api := makeAPI(c)91 api := makeAPI(c)
92 diskName := "gwacldiske5w7lkj"92 diskName := "gwacldiske5w7lkj"
93 poller := diskDeletePoller{api, diskName}93 poller := diskDeletePoller{api, diskName, false}
9494
95 response, err := performPolling(poller, time.Nanosecond, time.Minute)95 response, err := performPolling(poller, time.Nanosecond, time.Minute)
9696
9797
=== 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 @@
158 sort.Strings(diskNames)158 sort.Strings(diskNames)
159 // 3. Delete the disks.159 // 3. Delete the disks.
160 for _, diskName := range diskNames {160 for _, diskName := range diskNames {
161 err = api.DeleteDisk(diskName)161 err = api.DeleteDisk(&DeleteDiskRequest{
162 DiskName: diskName,
163 DeleteBlob: false}) // change to true at some point?
162 if err != nil && !IsNotFoundError(err) {164 if err != nil && !IsNotFoundError(err) {
163 return err165 return err
164 }166 }
165167
=== 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 @@
290 return &keys, nil290 return &keys, nil
291}291}
292292
293type DeleteDiskRequest struct {
294 DiskName string // Name of the disk to delete.
295 DeleteBlob bool // Whether to delete the associated blob storage.
296}
297
293// DeleteDisk deletes the named OS/data disk.298// DeleteDisk deletes the named OS/data disk.
294// See http://msdn.microsoft.com/en-us/library/windowsazure/jj157200.aspx299// See http://msdn.microsoft.com/en-us/library/windowsazure/jj157200.aspx
295// TODO: set comp=media to optionally delete associated blob.300func (api *ManagementAPI) DeleteDisk(request *DeleteDiskRequest) error {
296func (api *ManagementAPI) DeleteDisk(diskName string) error {
297 // Use the disk deletion poller to work around a bug in Windows Azure.301 // Use the disk deletion poller to work around a bug in Windows Azure.
298 // See the documentation in the file deletedisk.go for details.302 // See the documentation in the file deletedisk.go for details.
299 poller := diskDeletePoller{api, diskName}303 poller := diskDeletePoller{api, request.DiskName, request.DeleteBlob}
300 _, err := performPolling(poller, deleteDiskInterval, deleteDiskTimeout)304 _, err := performPolling(poller, deleteDiskInterval, deleteDiskTimeout)
301 return err305 return err
302}306}
303307
304func (api *ManagementAPI) _DeleteDisk(diskName string) error {308func (api *ManagementAPI) _DeleteDisk(diskName string, deleteBlob bool) error {
305 response, err := api.session.delete(309 url := "services/disks/" + diskName
306 "services/disks/"+diskName, "2012-08-01")310 if deleteBlob {
311 url = addURLQueryParams(url, "comp", "media")
312 }
313 response, err := api.session.delete(url, "2012-08-01")
307 if err != nil {314 if err != nil {
308 return err315 return err
309 }316 }
310317
=== 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 @@
10 "fmt"10 "fmt"
11 . "launchpad.net/gocheck"11 . "launchpad.net/gocheck"
12 "net/http"12 "net/http"
13 "net/url"
13 "strings"14 "strings"
14 "time"15 "time"
15)16)
@@ -754,13 +755,32 @@
754 api := makeAPI(c)755 api := makeAPI(c)
755 recordedRequests := setUpDispatcher("operationID")756 recordedRequests := setUpDispatcher("operationID")
756 diskName := "diskName"757 diskName := "diskName"
757 err := api.DeleteDisk(diskName)758 err := api.DeleteDisk(&DeleteDiskRequest{DiskName: diskName})
758759
759 c.Assert(err, IsNil)760 c.Assert(err, IsNil)
760 c.Assert(*recordedRequests, HasLen, 1)761 c.Assert(*recordedRequests, HasLen, 1)
761 assertDeleteDiskRequest(c, api, diskName, (*recordedRequests)[0])762 assertDeleteDiskRequest(c, api, diskName, (*recordedRequests)[0])
762}763}
763764
765func (suite *managementBaseAPISuite) TestDeleteDiskWithDeleteBlob(c *C) {
766 // Setting deleteBlob=true should append comp=media as a url query value.
767 deleteDiskInterval = time.Nanosecond
768 api := makeAPI(c)
769 recordedRequests := setUpDispatcher("operationID")
770 diskName := "diskName"
771
772 err := api.DeleteDisk(&DeleteDiskRequest{
773 DiskName: diskName, DeleteBlob: true})
774
775 c.Assert(err, IsNil)
776 originalURL := (*recordedRequests)[0].URL
777 parsedURL, err := url.Parse(originalURL)
778 c.Assert(err, IsNil)
779 values := parsedURL.Query()
780 c.Assert(err, IsNil)
781 c.Check(values["comp"], DeepEquals, []string{"media"})
782}
783
764func (suite *managementBaseAPISuite) TestPerformNodeOperation(c *C) {784func (suite *managementBaseAPISuite) TestPerformNodeOperation(c *C) {
765 api := makeAPI(c)785 api := makeAPI(c)
766 recordedRequests := setUpDispatcher("operationID")786 recordedRequests := setUpDispatcher("operationID")

Subscribers

People subscribed via source and target branches

to all changes: