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

Proposed by Julian Edwards on 2013-07-16
Status: Merged
Approved by: Julian Edwards on 2013-07-17
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) 2013-07-16 Approve on 2013-07-16
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 on 2013-07-16

format

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 on 2013-07-17

Make DeleteDisk take a request struct param

190. By Julian Edwards on 2013-07-17

FORMAT

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
1=== modified file 'deletedisk.go'
2--- deletedisk.go 2013-04-17 08:10:24 +0000
3+++ deletedisk.go 2013-07-17 01:01:23 +0000
4@@ -27,14 +27,15 @@
5 var deleteDiskInterval = 10 * time.Second
6
7 type diskDeletePoller struct {
8- api *ManagementAPI
9- diskName string
10+ api *ManagementAPI
11+ diskName string
12+ deleteBlob bool
13 }
14
15 var _ poller = &diskDeletePoller{}
16
17 func (poller diskDeletePoller) poll() (*x509Response, error) {
18- return nil, poller.api._DeleteDisk(poller.diskName)
19+ return nil, poller.api._DeleteDisk(poller.diskName, poller.deleteBlob)
20 }
21
22 // isInUseError returns whether or not the given string is of the "disk in use"
23
24=== modified file 'deletedisk_test.go'
25--- deletedisk_test.go 2013-07-12 04:22:16 +0000
26+++ deletedisk_test.go 2013-07-17 01:01:23 +0000
27@@ -38,7 +38,7 @@
28 }
29
30 func (suite *deleteDiskSuite) TestIsDoneReturnsTrueIfNilError(c *C) {
31- poller := diskDeletePoller{nil, ""}
32+ poller := diskDeletePoller{nil, "", false}
33 randomResponse := x509Response{StatusCode: http.StatusAccepted}
34 done, err := poller.isDone(&randomResponse, nil)
35 c.Check(done, Equals, true)
36@@ -48,7 +48,7 @@
37 func (suite *deleteDiskSuite) TestIsDoneReturnsFalseIfDiskInUseError(c *C) {
38 diskName := "gwacldiske5w7lkj"
39 diskInUseError := fmt.Errorf(diskInUseErrorTemplate, diskName)
40- poller := diskDeletePoller{nil, diskName}
41+ poller := diskDeletePoller{nil, diskName, false}
42 done, err := poller.isDone(nil, diskInUseError)
43 c.Check(done, Equals, false)
44 c.Check(err, IsNil)
45@@ -56,7 +56,7 @@
46
47 func (suite *deleteDiskSuite) TestIsDoneReturnsTrueIfAnotherError(c *C) {
48 anotherError := fmt.Errorf("Unknown error")
49- poller := diskDeletePoller{nil, "disk-name"}
50+ poller := diskDeletePoller{nil, "disk-name", false}
51 done, err := poller.isDone(nil, anotherError)
52 c.Check(done, Equals, true)
53 c.Check(err, Equals, anotherError)
54@@ -66,7 +66,7 @@
55 api := makeAPI(c)
56 recordedRequests := setUpDispatcher("operationID")
57 diskName := "gwacldiske5w7lkj"
58- poller := diskDeletePoller{api, diskName}
59+ poller := diskDeletePoller{api, diskName, false}
60
61 response, err := poller.poll()
62
63@@ -90,7 +90,7 @@
64
65 api := makeAPI(c)
66 diskName := "gwacldiske5w7lkj"
67- poller := diskDeletePoller{api, diskName}
68+ poller := diskDeletePoller{api, diskName, false}
69
70 response, err := performPolling(poller, time.Nanosecond, time.Minute)
71
72
73=== modified file 'management.go'
74--- management.go 2013-07-12 12:55:23 +0000
75+++ management.go 2013-07-17 01:01:23 +0000
76@@ -158,7 +158,9 @@
77 sort.Strings(diskNames)
78 // 3. Delete the disks.
79 for _, diskName := range diskNames {
80- err = api.DeleteDisk(diskName)
81+ err = api.DeleteDisk(&DeleteDiskRequest{
82+ DiskName: diskName,
83+ DeleteBlob: false}) // change to true at some point?
84 if err != nil && !IsNotFoundError(err) {
85 return err
86 }
87
88=== modified file 'management_base.go'
89--- management_base.go 2013-07-16 15:43:48 +0000
90+++ management_base.go 2013-07-17 01:01:23 +0000
91@@ -290,20 +290,27 @@
92 return &keys, nil
93 }
94
95+type DeleteDiskRequest struct {
96+ DiskName string // Name of the disk to delete.
97+ DeleteBlob bool // Whether to delete the associated blob storage.
98+}
99+
100 // DeleteDisk deletes the named OS/data disk.
101 // See http://msdn.microsoft.com/en-us/library/windowsazure/jj157200.aspx
102-// TODO: set comp=media to optionally delete associated blob.
103-func (api *ManagementAPI) DeleteDisk(diskName string) error {
104+func (api *ManagementAPI) DeleteDisk(request *DeleteDiskRequest) error {
105 // Use the disk deletion poller to work around a bug in Windows Azure.
106 // See the documentation in the file deletedisk.go for details.
107- poller := diskDeletePoller{api, diskName}
108+ poller := diskDeletePoller{api, request.DiskName, request.DeleteBlob}
109 _, err := performPolling(poller, deleteDiskInterval, deleteDiskTimeout)
110 return err
111 }
112
113-func (api *ManagementAPI) _DeleteDisk(diskName string) error {
114- response, err := api.session.delete(
115- "services/disks/"+diskName, "2012-08-01")
116+func (api *ManagementAPI) _DeleteDisk(diskName string, deleteBlob bool) error {
117+ url := "services/disks/" + diskName
118+ if deleteBlob {
119+ url = addURLQueryParams(url, "comp", "media")
120+ }
121+ response, err := api.session.delete(url, "2012-08-01")
122 if err != nil {
123 return err
124 }
125
126=== modified file 'management_base_test.go'
127--- management_base_test.go 2013-07-16 15:43:48 +0000
128+++ management_base_test.go 2013-07-17 01:01:23 +0000
129@@ -10,6 +10,7 @@
130 "fmt"
131 . "launchpad.net/gocheck"
132 "net/http"
133+ "net/url"
134 "strings"
135 "time"
136 )
137@@ -754,13 +755,32 @@
138 api := makeAPI(c)
139 recordedRequests := setUpDispatcher("operationID")
140 diskName := "diskName"
141- err := api.DeleteDisk(diskName)
142+ err := api.DeleteDisk(&DeleteDiskRequest{DiskName: diskName})
143
144 c.Assert(err, IsNil)
145 c.Assert(*recordedRequests, HasLen, 1)
146 assertDeleteDiskRequest(c, api, diskName, (*recordedRequests)[0])
147 }
148
149+func (suite *managementBaseAPISuite) TestDeleteDiskWithDeleteBlob(c *C) {
150+ // Setting deleteBlob=true should append comp=media as a url query value.
151+ deleteDiskInterval = time.Nanosecond
152+ api := makeAPI(c)
153+ recordedRequests := setUpDispatcher("operationID")
154+ diskName := "diskName"
155+
156+ err := api.DeleteDisk(&DeleteDiskRequest{
157+ DiskName: diskName, DeleteBlob: true})
158+
159+ c.Assert(err, IsNil)
160+ originalURL := (*recordedRequests)[0].URL
161+ parsedURL, err := url.Parse(originalURL)
162+ c.Assert(err, IsNil)
163+ values := parsedURL.Query()
164+ c.Assert(err, IsNil)
165+ c.Check(values["comp"], DeepEquals, []string{"media"})
166+}
167+
168 func (suite *managementBaseAPISuite) TestPerformNodeOperation(c *C) {
169 api := makeAPI(c)
170 recordedRequests := setUpDispatcher("operationID")

Subscribers

People subscribed via source and target branches

to all changes: