Merge lp:~allenap/gwacl/add-virtual-network into lp:gwacl
- add-virtual-network
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Gavin Panella |
Approved revision: | 206 |
Merged at revision: | 180 |
Proposed branch: | lp:~allenap/gwacl/add-virtual-network |
Merge into: | lp:gwacl |
Diff against target: |
427 lines (+337/-24) 5 files modified
example/management/run.go (+9/-21) management.go (+53/-0) management_base_test.go (+11/-2) management_test.go (+264/-0) xmlobjects.go (+0/-1) |
To merge this branch: | bzr merge lp:~allenap/gwacl/add-virtual-network |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Raphaël Badin (community) | Approve | ||
Review via email: mp+174382@code.launchpad.net |
Commit message
Two new methods, {Add,Remove}
Description of the change
- 202. By Gavin Panella
-
Remove early return.
- 203. By Gavin Panella
-
Simplify AddVirtualNetwo
rkSite (courtesy of jtv). - 204. By Gavin Panella
-
Throw a wobbly when a virtual network already exists.
- 205. By Gavin Panella
-
Merge trunk, resolving one conflict.
- 206. By Gavin Panella
-
Add some whitespace.
Gavin Panella (allenap) wrote : | # |
> [0]
>
> 76 + for _, existingSite := range *networkConfig.
> 77 + if existingSite.Name == site.Name {
> 78 + // Network already defined.
> 79 + return nil
> 80 + }
>
> That's a weird behavior don't you think? Say I try adding a network with the
> same name but a different affinity group; the method will return with a nil
> error so I'll think that everything is ok and it will be a lie :). I /think/
> returning an error (maybe only if the network you're trying to add has exactly
> the same characteristics as the one with the same name which already exists)
> is more appropriate here. What do you think?
Good point. I've changed it to return an error when there's a
preexisting network with the same name.
>
> [1]
>
> 102 + for _, existingSite := range *networkConfig.
> 103 + if existingSite.Name != siteName {
> 104 + virtualNetworkSites = append(
> existingSite)
> 105 + }
>
> We cannot just remove the network in question from
> networkConfig.
Is there a reason why we can't?
Ah, do you mean why can't we delete the entry directly from the slice?
Because Go.
Seriously, if you know how to do this nicely, please let me know.
>
> [2]
>
> 208 + virtualNetwork := &VirtualNetwork
> MakeRandomVirtu
> 209 + err := api.AddVirtualN
> 210 + c.Assert(err, IsNil)
> 211 + c.Check(record, HasLen, 2)
>
> It's a detail but that's something Jeroen and I have tried to do in tests,
> especially when the tests are long: put a empty line between the
> initialization code and the method you're testing, and another after that.
> This way, we have 3 visually well-separated blocks: initialization, method
> call, checks.
It's a pretty inconsistently followed pattern, but what the hell;
fixed.
Fwiw, I've been avoiding blank lines because gof*t only adds a single
line between functions instead of the two I'm used to.
>
> [3]
>
> You could add a check in example/
> created machine effectively has an IP in the virtual network.
I'll give this a go.
Gavin Panella (allenap) wrote : | # |
> > [3]
> >
> > You could add a check in example/
> > created machine effectively has an IP in the virtual network.
>
> I'll give this a go.
I've just realised there's a good reason why it doesn't do this: VMs
are not yet deployed into the virtual network that's created. That's
something for a subsequent branch.
Raphaël Badin (rvb) wrote : | # |
> > > [3]
> > >
> > > You could add a check in example/
> > > created machine effectively has an IP in the virtual network.
> >
> > I'll give this a go.
>
> I've just realised there's a good reason why it doesn't do this: VMs
> are not yet deployed into the virtual network that's created. That's
> something for a subsequent branch.
Fair enough… it should be as simple as giving the name of the vnet to gwacl.NewDeploy
Raphaël Badin (rvb) wrote : | # |
> > Ah, do you mean why can't we delete the entry directly from the slice?
> Because Go.
Yeah, because Go :/
> Seriously, if you know how to do this nicely, please let me know.
I don't… that's a shame.
Raphaël Badin (rvb) wrote : | # |
Thanks for the fixes.
Gavin Panella (allenap) wrote : | # |
Thanks for the review!
Preview Diff
1 | === modified file 'example/management/run.go' | |||
2 | --- example/management/run.go 2013-07-10 16:59:00 +0000 | |||
3 | +++ example/management/run.go 2013-07-12 13:07:30 +0000 | |||
4 | @@ -108,33 +108,21 @@ | |||
5 | 108 | 108 | ||
6 | 109 | virtualNetworkName := gwacl.MakeRandomVirtualNetworkName("virtual-net-") | 109 | virtualNetworkName := gwacl.MakeRandomVirtualNetworkName("virtual-net-") |
7 | 110 | fmt.Printf("Creating virtual network %s...\n", virtualNetworkName) | 110 | fmt.Printf("Creating virtual network %s...\n", virtualNetworkName) |
20 | 111 | // FIXME: This clobbers *all* networking configuration instead of just | 111 | virtualNetwork := gwacl.VirtualNetworkSite{ |
21 | 112 | // adding the specific virtual network. | 112 | Name: virtualNetworkName, |
22 | 113 | vnetRequest := &gwacl.NetworkConfiguration{ | 113 | AffinityGroup: affinityGroupName, |
23 | 114 | XMLNS: gwacl.XMLNS_NC, | 114 | AddressSpacePrefixes: []string{ |
24 | 115 | VirtualNetworkSites: &[]gwacl.VirtualNetworkSite{ | 115 | "10.0.0.0/8", |
13 | 116 | { | ||
14 | 117 | Name: virtualNetworkName, | ||
15 | 118 | AffinityGroup: affinityGroupName, | ||
16 | 119 | AddressSpacePrefixes: []string{ | ||
17 | 120 | "10.0.0.0/8", | ||
18 | 121 | }, | ||
19 | 122 | }, | ||
25 | 123 | }, | 116 | }, |
26 | 124 | } | 117 | } |
28 | 125 | err = api.SetNetworkConfiguration(vnetRequest) | 118 | err = api.AddVirtualNetworkSite(&virtualNetwork) |
29 | 126 | checkError(err) | 119 | checkError(err) |
30 | 127 | fmt.Println("Done creating virtual network") | 120 | fmt.Println("Done creating virtual network") |
31 | 128 | 121 | ||
32 | 129 | defer func() { | 122 | defer func() { |
41 | 130 | fmt.Println("Deleting virtual network...") | 123 | fmt.Printf("Deleting virtual network %s...\n", virtualNetworkName) |
42 | 131 | // FIXME: This destroys *all* networking configuration instead of just | 124 | api.RemoveVirtualNetworkSite(virtualNetworkName) |
43 | 132 | // removing the specific network we created. | 125 | fmt.Printf("Done deleting virtual network %s\n", virtualNetworkName) |
36 | 133 | api.SetNetworkConfiguration(&gwacl.NetworkConfiguration{ | ||
37 | 134 | XMLNS: gwacl.XMLNS_NC, | ||
38 | 135 | VirtualNetworkSites: &[]gwacl.VirtualNetworkSite{}, | ||
39 | 136 | }) | ||
40 | 137 | fmt.Println("Done deleting virtual network") | ||
44 | 138 | }() | 126 | }() |
45 | 139 | 127 | ||
46 | 140 | networkConfig, err := api.GetNetworkConfiguration() | 128 | networkConfig, err := api.GetNetworkConfiguration() |
47 | 141 | 129 | ||
48 | === modified file 'management.go' | |||
49 | --- management.go 2013-07-08 07:11:29 +0000 | |||
50 | +++ management.go 2013-07-12 13:07:30 +0000 | |||
51 | @@ -4,6 +4,7 @@ | |||
52 | 4 | package gwacl | 4 | package gwacl |
53 | 5 | 5 | ||
54 | 6 | import ( | 6 | import ( |
55 | 7 | "fmt" | ||
56 | 7 | "sort" | 8 | "sort" |
57 | 8 | "strings" | 9 | "strings" |
58 | 9 | ) | 10 | ) |
59 | @@ -199,3 +200,55 @@ | |||
60 | 199 | // Done. | 200 | // Done. |
61 | 200 | return nil | 201 | return nil |
62 | 201 | } | 202 | } |
63 | 203 | |||
64 | 204 | func (api *ManagementAPI) AddVirtualNetworkSite(site *VirtualNetworkSite) error { | ||
65 | 205 | // Obtain the current network config, which we will then modify. | ||
66 | 206 | networkConfig, err := api.GetNetworkConfiguration() | ||
67 | 207 | if err != nil { | ||
68 | 208 | return err | ||
69 | 209 | } | ||
70 | 210 | if networkConfig == nil { | ||
71 | 211 | // There's no config yet. | ||
72 | 212 | networkConfig = &NetworkConfiguration{XMLNS: XMLNS_NC} | ||
73 | 213 | } | ||
74 | 214 | if networkConfig.VirtualNetworkSites == nil { | ||
75 | 215 | networkConfig.VirtualNetworkSites = &[]VirtualNetworkSite{} | ||
76 | 216 | } | ||
77 | 217 | // Check to see if this network already exists. | ||
78 | 218 | for _, existingSite := range *networkConfig.VirtualNetworkSites { | ||
79 | 219 | if existingSite.Name == site.Name { | ||
80 | 220 | // Network already defined. | ||
81 | 221 | return fmt.Errorf("could not add virtual network: %q already exists", site.Name) | ||
82 | 222 | } | ||
83 | 223 | } | ||
84 | 224 | // Add the network to the configuration. | ||
85 | 225 | virtualNetworkSites := append(*networkConfig.VirtualNetworkSites, *site) | ||
86 | 226 | networkConfig.VirtualNetworkSites = &virtualNetworkSites | ||
87 | 227 | // Put it back up to Azure. There's a race here... | ||
88 | 228 | return api.SetNetworkConfiguration(networkConfig) | ||
89 | 229 | } | ||
90 | 230 | |||
91 | 231 | func (api *ManagementAPI) RemoveVirtualNetworkSite(siteName string) error { | ||
92 | 232 | // Obtain the current network config, which we will then modify. | ||
93 | 233 | networkConfig, err := api.GetNetworkConfiguration() | ||
94 | 234 | if err != nil { | ||
95 | 235 | return err | ||
96 | 236 | } | ||
97 | 237 | if networkConfig == nil || networkConfig.VirtualNetworkSites == nil { | ||
98 | 238 | // There's no config, nothing to do. | ||
99 | 239 | return nil | ||
100 | 240 | } | ||
101 | 241 | // Remove all references to the specified virtual network site name. | ||
102 | 242 | virtualNetworkSites := []VirtualNetworkSite{} | ||
103 | 243 | for _, existingSite := range *networkConfig.VirtualNetworkSites { | ||
104 | 244 | if existingSite.Name != siteName { | ||
105 | 245 | virtualNetworkSites = append(virtualNetworkSites, existingSite) | ||
106 | 246 | } | ||
107 | 247 | } | ||
108 | 248 | if len(virtualNetworkSites) < len(*networkConfig.VirtualNetworkSites) { | ||
109 | 249 | // Put it back up to Azure. There's a race here... | ||
110 | 250 | networkConfig.VirtualNetworkSites = &virtualNetworkSites | ||
111 | 251 | return api.SetNetworkConfiguration(networkConfig) | ||
112 | 252 | } | ||
113 | 253 | return nil | ||
114 | 254 | } | ||
115 | 202 | 255 | ||
116 | === modified file 'management_base_test.go' | |||
117 | --- management_base_test.go 2013-07-12 04:45:34 +0000 | |||
118 | +++ management_base_test.go 2013-07-12 13:07:30 +0000 | |||
119 | @@ -1001,6 +1001,15 @@ | |||
120 | 1001 | assertGetNetworkConfigurationRequest(c, api, recordedRequests[0]) | 1001 | assertGetNetworkConfigurationRequest(c, api, recordedRequests[0]) |
121 | 1002 | } | 1002 | } |
122 | 1003 | 1003 | ||
123 | 1004 | func assertSetNetworkConfigurationRequest(c *C, api *ManagementAPI, body []byte, httpRequest *X509Request) { | ||
124 | 1005 | expectedURL := fmt.Sprintf( | ||
125 | 1006 | "%s%s/services/networking/media", AZURE_URL, | ||
126 | 1007 | api.session.subscriptionId) | ||
127 | 1008 | checkRequest(c, httpRequest, expectedURL, "2012-03-01", body, "PUT") | ||
128 | 1009 | // Azure chokes when the content type is text/xml or similar. | ||
129 | 1010 | c.Assert(httpRequest.ContentType, Equals, "application/octet-stream") | ||
130 | 1011 | } | ||
131 | 1012 | |||
132 | 1004 | func (suite *managementBaseAPISuite) TestSetNetworkConfiguration(c *C) { | 1013 | func (suite *managementBaseAPISuite) TestSetNetworkConfiguration(c *C) { |
133 | 1005 | api := makeAPI(c) | 1014 | api := makeAPI(c) |
134 | 1006 | fixedResponse := x509Response{StatusCode: http.StatusOK} | 1015 | fixedResponse := x509Response{StatusCode: http.StatusOK} |
135 | @@ -1015,6 +1024,6 @@ | |||
136 | 1015 | err = api.SetNetworkConfiguration(request) | 1024 | err = api.SetNetworkConfiguration(request) |
137 | 1016 | 1025 | ||
138 | 1017 | c.Assert(err, IsNil) | 1026 | c.Assert(err, IsNil) |
141 | 1018 | expectedURL := AZURE_URL + api.session.subscriptionId + "/services/networking/media" | 1027 | c.Assert(recordedRequests, HasLen, 1) |
142 | 1019 | checkOneRequest(c, &recordedRequests, expectedURL, "2012-03-01", requestPayload, "PUT") | 1028 | assertSetNetworkConfigurationRequest(c, api, requestPayload, recordedRequests[0]) |
143 | 1020 | } | 1029 | } |
144 | 1021 | 1030 | ||
145 | === modified file 'management_test.go' | |||
146 | --- management_test.go 2013-07-12 04:22:16 +0000 | |||
147 | +++ management_test.go 2013-07-12 13:07:30 +0000 | |||
148 | @@ -579,3 +579,267 @@ | |||
149 | 579 | c.Check(err, ErrorMatches, "DELETE request failed [(]500: Internal Server Error[)]") | 579 | c.Check(err, ErrorMatches, "DELETE request failed [(]500: Internal Server Error[)]") |
150 | 580 | c.Check(record, HasLen, 4) | 580 | c.Check(record, HasLen, 4) |
151 | 581 | } | 581 | } |
152 | 582 | |||
153 | 583 | type suiteAddVirtualNetworkSite struct{} | ||
154 | 584 | |||
155 | 585 | var _ = Suite(&suiteAddVirtualNetworkSite{}) | ||
156 | 586 | |||
157 | 587 | func (suite *suiteAddVirtualNetworkSite) TestWhenConfigCannotBeFetched(c *C) { | ||
158 | 588 | responses := []DispatcherResponse{ | ||
159 | 589 | {response: &x509Response{StatusCode: http.StatusInternalServerError}}, | ||
160 | 590 | } | ||
161 | 591 | record := []*X509Request{} | ||
162 | 592 | rigRecordingPreparedResponseDispatcher(&record, responses) | ||
163 | 593 | api := makeAPI(c) | ||
164 | 594 | |||
165 | 595 | err := api.AddVirtualNetworkSite(nil) | ||
166 | 596 | |||
167 | 597 | c.Assert(err, NotNil) | ||
168 | 598 | c.Check(err, ErrorMatches, "GET request failed [(]500: Internal Server Error[)]") | ||
169 | 599 | c.Check(record, HasLen, 1) | ||
170 | 600 | assertGetNetworkConfigurationRequest(c, api, record[0]) | ||
171 | 601 | } | ||
172 | 602 | |||
173 | 603 | func (suite *suiteAddVirtualNetworkSite) TestWhenConfigDoesNotExist(c *C) { | ||
174 | 604 | responses := []DispatcherResponse{ | ||
175 | 605 | // No configuration found. | ||
176 | 606 | {response: &x509Response{StatusCode: http.StatusNotFound}}, | ||
177 | 607 | // Accept upload of new configuration. | ||
178 | 608 | {response: &x509Response{StatusCode: http.StatusOK}}, | ||
179 | 609 | } | ||
180 | 610 | record := []*X509Request{} | ||
181 | 611 | rigRecordingPreparedResponseDispatcher(&record, responses) | ||
182 | 612 | api := makeAPI(c) | ||
183 | 613 | virtualNetwork := &VirtualNetworkSite{Name: MakeRandomVirtualNetworkName("test-")} | ||
184 | 614 | |||
185 | 615 | err := api.AddVirtualNetworkSite(virtualNetwork) | ||
186 | 616 | |||
187 | 617 | c.Assert(err, IsNil) | ||
188 | 618 | c.Check(record, HasLen, 2) | ||
189 | 619 | assertGetNetworkConfigurationRequest(c, api, record[0]) | ||
190 | 620 | expected := &NetworkConfiguration{ | ||
191 | 621 | XMLNS: XMLNS_NC, | ||
192 | 622 | VirtualNetworkSites: &[]VirtualNetworkSite{*virtualNetwork}, | ||
193 | 623 | } | ||
194 | 624 | expectedBody, err := expected.Serialize() | ||
195 | 625 | c.Assert(err, IsNil) | ||
196 | 626 | assertSetNetworkConfigurationRequest(c, api, []byte(expectedBody), record[1]) | ||
197 | 627 | } | ||
198 | 628 | |||
199 | 629 | func (suite *suiteAddVirtualNetworkSite) TestWhenNoPreexistingVirtualNetworkSites(c *C) { | ||
200 | 630 | // Prepare a basic, empty, configuration. | ||
201 | 631 | existingConfig := &NetworkConfiguration{XMLNS: XMLNS_NC} | ||
202 | 632 | responses := makeOKXMLResponse(c, existingConfig) | ||
203 | 633 | responses = append(responses, DispatcherResponse{ | ||
204 | 634 | // Accept upload of new configuration. | ||
205 | 635 | response: &x509Response{StatusCode: http.StatusOK}, | ||
206 | 636 | }) | ||
207 | 637 | record := []*X509Request{} | ||
208 | 638 | rigRecordingPreparedResponseDispatcher(&record, responses) | ||
209 | 639 | api := makeAPI(c) | ||
210 | 640 | virtualNetwork := &VirtualNetworkSite{Name: MakeRandomVirtualNetworkName("test-")} | ||
211 | 641 | |||
212 | 642 | err := api.AddVirtualNetworkSite(virtualNetwork) | ||
213 | 643 | |||
214 | 644 | c.Assert(err, IsNil) | ||
215 | 645 | c.Check(record, HasLen, 2) | ||
216 | 646 | assertGetNetworkConfigurationRequest(c, api, record[0]) | ||
217 | 647 | expected := &NetworkConfiguration{ | ||
218 | 648 | XMLNS: XMLNS_NC, | ||
219 | 649 | VirtualNetworkSites: &[]VirtualNetworkSite{*virtualNetwork}, | ||
220 | 650 | } | ||
221 | 651 | expectedBody, err := expected.Serialize() | ||
222 | 652 | c.Assert(err, IsNil) | ||
223 | 653 | assertSetNetworkConfigurationRequest(c, api, []byte(expectedBody), record[1]) | ||
224 | 654 | } | ||
225 | 655 | |||
226 | 656 | func (suite *suiteAddVirtualNetworkSite) TestWhenPreexistingVirtualNetworkSites(c *C) { | ||
227 | 657 | // Prepare a configuration with a single virtual network. | ||
228 | 658 | existingConfig := &NetworkConfiguration{ | ||
229 | 659 | XMLNS: XMLNS_NC, | ||
230 | 660 | VirtualNetworkSites: &[]VirtualNetworkSite{ | ||
231 | 661 | {Name: MakeRandomVirtualNetworkName("test-")}, | ||
232 | 662 | }, | ||
233 | 663 | } | ||
234 | 664 | responses := makeOKXMLResponse(c, existingConfig) | ||
235 | 665 | responses = append(responses, DispatcherResponse{ | ||
236 | 666 | response: &x509Response{StatusCode: http.StatusOK}, | ||
237 | 667 | }) | ||
238 | 668 | record := []*X509Request{} | ||
239 | 669 | rigRecordingPreparedResponseDispatcher(&record, responses) | ||
240 | 670 | api := makeAPI(c) | ||
241 | 671 | virtualNetwork := &VirtualNetworkSite{Name: MakeRandomVirtualNetworkName("test-")} | ||
242 | 672 | |||
243 | 673 | err := api.AddVirtualNetworkSite(virtualNetwork) | ||
244 | 674 | |||
245 | 675 | c.Assert(err, IsNil) | ||
246 | 676 | c.Check(record, HasLen, 2) | ||
247 | 677 | assertGetNetworkConfigurationRequest(c, api, record[0]) | ||
248 | 678 | expectedSites := append( | ||
249 | 679 | *existingConfig.VirtualNetworkSites, *virtualNetwork) | ||
250 | 680 | expected := &NetworkConfiguration{ | ||
251 | 681 | XMLNS: XMLNS_NC, | ||
252 | 682 | VirtualNetworkSites: &expectedSites, | ||
253 | 683 | } | ||
254 | 684 | expectedBody, err := expected.Serialize() | ||
255 | 685 | c.Assert(err, IsNil) | ||
256 | 686 | assertSetNetworkConfigurationRequest(c, api, []byte(expectedBody), record[1]) | ||
257 | 687 | } | ||
258 | 688 | |||
259 | 689 | func (suite *suiteAddVirtualNetworkSite) TestWhenPreexistingVirtualNetworkSiteWithSameName(c *C) { | ||
260 | 690 | // Prepare a configuration with a single virtual network. | ||
261 | 691 | existingConfig := &NetworkConfiguration{ | ||
262 | 692 | XMLNS: XMLNS_NC, | ||
263 | 693 | VirtualNetworkSites: &[]VirtualNetworkSite{ | ||
264 | 694 | {Name: "virtual-network-bob"}, | ||
265 | 695 | }, | ||
266 | 696 | } | ||
267 | 697 | responses := makeOKXMLResponse(c, existingConfig) | ||
268 | 698 | rigPreparedResponseDispatcher(responses) | ||
269 | 699 | api := makeAPI(c) | ||
270 | 700 | virtualNetwork := &VirtualNetworkSite{Name: "virtual-network-bob"} | ||
271 | 701 | |||
272 | 702 | err := api.AddVirtualNetworkSite(virtualNetwork) | ||
273 | 703 | |||
274 | 704 | c.Check(err, ErrorMatches, "could not add virtual network: \"virtual-network-bob\" already exists") | ||
275 | 705 | } | ||
276 | 706 | |||
277 | 707 | func (suite *suiteAddVirtualNetworkSite) TestWhenConfigCannotBePushed(c *C) { | ||
278 | 708 | responses := []DispatcherResponse{ | ||
279 | 709 | // No configuration found. | ||
280 | 710 | {response: &x509Response{StatusCode: http.StatusNotFound}}, | ||
281 | 711 | // Cannot accept upload of new configuration. | ||
282 | 712 | {response: &x509Response{StatusCode: http.StatusInternalServerError}}, | ||
283 | 713 | } | ||
284 | 714 | rigPreparedResponseDispatcher(responses) | ||
285 | 715 | virtualNetwork := &VirtualNetworkSite{Name: MakeRandomVirtualNetworkName("test-")} | ||
286 | 716 | |||
287 | 717 | err := makeAPI(c).AddVirtualNetworkSite(virtualNetwork) | ||
288 | 718 | |||
289 | 719 | c.Assert(err, NotNil) | ||
290 | 720 | c.Check(err, ErrorMatches, "PUT request failed [(]500: Internal Server Error[)]") | ||
291 | 721 | } | ||
292 | 722 | |||
293 | 723 | type suiteRemoveVirtualNetworkSite struct{} | ||
294 | 724 | |||
295 | 725 | var _ = Suite(&suiteRemoveVirtualNetworkSite{}) | ||
296 | 726 | |||
297 | 727 | func (suite *suiteRemoveVirtualNetworkSite) TestWhenConfigCannotBeFetched(c *C) { | ||
298 | 728 | responses := []DispatcherResponse{ | ||
299 | 729 | {response: &x509Response{StatusCode: http.StatusInternalServerError}}, | ||
300 | 730 | } | ||
301 | 731 | record := []*X509Request{} | ||
302 | 732 | rigRecordingPreparedResponseDispatcher(&record, responses) | ||
303 | 733 | api := makeAPI(c) | ||
304 | 734 | |||
305 | 735 | err := api.RemoveVirtualNetworkSite("virtual-network-o-doom") | ||
306 | 736 | |||
307 | 737 | c.Check(err, ErrorMatches, "GET request failed [(]500: Internal Server Error[)]") | ||
308 | 738 | c.Check(record, HasLen, 1) | ||
309 | 739 | assertGetNetworkConfigurationRequest(c, api, record[0]) | ||
310 | 740 | } | ||
311 | 741 | |||
312 | 742 | func (suite *suiteRemoveVirtualNetworkSite) TestWhenConfigDoesNotExist(c *C) { | ||
313 | 743 | responses := []DispatcherResponse{ | ||
314 | 744 | {response: &x509Response{StatusCode: http.StatusNotFound}}, | ||
315 | 745 | } | ||
316 | 746 | record := []*X509Request{} | ||
317 | 747 | rigRecordingPreparedResponseDispatcher(&record, responses) | ||
318 | 748 | api := makeAPI(c) | ||
319 | 749 | |||
320 | 750 | err := api.RemoveVirtualNetworkSite("virtual-network-in-my-eyes") | ||
321 | 751 | |||
322 | 752 | c.Assert(err, IsNil) | ||
323 | 753 | c.Check(record, HasLen, 1) | ||
324 | 754 | assertGetNetworkConfigurationRequest(c, api, record[0]) | ||
325 | 755 | } | ||
326 | 756 | |||
327 | 757 | func (suite *suiteRemoveVirtualNetworkSite) TestWhenNoPreexistingVirtualNetworkSites(c *C) { | ||
328 | 758 | // Prepare a basic, empty, configuration. | ||
329 | 759 | existingConfig := &NetworkConfiguration{XMLNS: XMLNS_NC} | ||
330 | 760 | responses := makeOKXMLResponse(c, existingConfig) | ||
331 | 761 | record := []*X509Request{} | ||
332 | 762 | rigRecordingPreparedResponseDispatcher(&record, responses) | ||
333 | 763 | api := makeAPI(c) | ||
334 | 764 | |||
335 | 765 | err := api.RemoveVirtualNetworkSite("virtual-network-on-my-shoes") | ||
336 | 766 | |||
337 | 767 | c.Assert(err, IsNil) | ||
338 | 768 | c.Check(record, HasLen, 1) | ||
339 | 769 | assertGetNetworkConfigurationRequest(c, api, record[0]) | ||
340 | 770 | } | ||
341 | 771 | |||
342 | 772 | func (suite *suiteRemoveVirtualNetworkSite) TestWhenPreexistingVirtualNetworkSites(c *C) { | ||
343 | 773 | // Prepare a configuration with a single virtual network. | ||
344 | 774 | existingConfig := &NetworkConfiguration{ | ||
345 | 775 | XMLNS: XMLNS_NC, | ||
346 | 776 | VirtualNetworkSites: &[]VirtualNetworkSite{ | ||
347 | 777 | {Name: MakeRandomVirtualNetworkName("test-")}, | ||
348 | 778 | }, | ||
349 | 779 | } | ||
350 | 780 | responses := makeOKXMLResponse(c, existingConfig) | ||
351 | 781 | responses = append(responses, DispatcherResponse{ | ||
352 | 782 | response: &x509Response{StatusCode: http.StatusOK}, | ||
353 | 783 | }) | ||
354 | 784 | record := []*X509Request{} | ||
355 | 785 | rigRecordingPreparedResponseDispatcher(&record, responses) | ||
356 | 786 | api := makeAPI(c) | ||
357 | 787 | virtualNetwork := &VirtualNetworkSite{Name: MakeRandomVirtualNetworkName("test-")} | ||
358 | 788 | |||
359 | 789 | err := api.RemoveVirtualNetworkSite(virtualNetwork.Name) | ||
360 | 790 | |||
361 | 791 | c.Assert(err, IsNil) | ||
362 | 792 | c.Check(record, HasLen, 1) | ||
363 | 793 | assertGetNetworkConfigurationRequest(c, api, record[0]) | ||
364 | 794 | // It didn't do anything, so no upload. | ||
365 | 795 | } | ||
366 | 796 | |||
367 | 797 | func (suite *suiteRemoveVirtualNetworkSite) TestWhenPreexistingVirtualNetworkSiteWithSameName(c *C) { | ||
368 | 798 | // Prepare a configuration with a single virtual network. | ||
369 | 799 | existingConfig := &NetworkConfiguration{ | ||
370 | 800 | XMLNS: XMLNS_NC, | ||
371 | 801 | VirtualNetworkSites: &[]VirtualNetworkSite{ | ||
372 | 802 | {Name: "virtual-network-bob"}, | ||
373 | 803 | }, | ||
374 | 804 | } | ||
375 | 805 | responses := makeOKXMLResponse(c, existingConfig) | ||
376 | 806 | responses = append(responses, DispatcherResponse{ | ||
377 | 807 | response: &x509Response{StatusCode: http.StatusOK}, | ||
378 | 808 | }) | ||
379 | 809 | record := []*X509Request{} | ||
380 | 810 | rigRecordingPreparedResponseDispatcher(&record, responses) | ||
381 | 811 | api := makeAPI(c) | ||
382 | 812 | virtualNetwork := &VirtualNetworkSite{Name: "virtual-network-bob"} | ||
383 | 813 | |||
384 | 814 | err := api.RemoveVirtualNetworkSite(virtualNetwork.Name) | ||
385 | 815 | |||
386 | 816 | c.Assert(err, IsNil) | ||
387 | 817 | c.Check(record, HasLen, 2) | ||
388 | 818 | assertGetNetworkConfigurationRequest(c, api, record[0]) | ||
389 | 819 | expected := &NetworkConfiguration{ | ||
390 | 820 | XMLNS: XMLNS_NC, | ||
391 | 821 | VirtualNetworkSites: &[]VirtualNetworkSite{}, | ||
392 | 822 | } | ||
393 | 823 | expectedBody, err := expected.Serialize() | ||
394 | 824 | c.Assert(err, IsNil) | ||
395 | 825 | assertSetNetworkConfigurationRequest(c, api, []byte(expectedBody), record[1]) | ||
396 | 826 | } | ||
397 | 827 | |||
398 | 828 | func (suite *suiteRemoveVirtualNetworkSite) TestWhenConfigCannotBePushed(c *C) { | ||
399 | 829 | existingConfig := &NetworkConfiguration{ | ||
400 | 830 | XMLNS: XMLNS_NC, | ||
401 | 831 | VirtualNetworkSites: &[]VirtualNetworkSite{ | ||
402 | 832 | {Name: "virtual-network-all-over"}, | ||
403 | 833 | }, | ||
404 | 834 | } | ||
405 | 835 | responses := makeOKXMLResponse(c, existingConfig) | ||
406 | 836 | responses = append(responses, DispatcherResponse{ | ||
407 | 837 | response: &x509Response{StatusCode: http.StatusInternalServerError}, | ||
408 | 838 | }) | ||
409 | 839 | rigPreparedResponseDispatcher(responses) | ||
410 | 840 | |||
411 | 841 | err := makeAPI(c).RemoveVirtualNetworkSite("virtual-network-all-over") | ||
412 | 842 | |||
413 | 843 | c.Assert(err, NotNil) | ||
414 | 844 | c.Check(err, ErrorMatches, "PUT request failed [(]500: Internal Server Error[)]") | ||
415 | 845 | } | ||
416 | 582 | 846 | ||
417 | === modified file 'xmlobjects.go' | |||
418 | --- xmlobjects.go 2013-07-11 03:41:33 +0000 | |||
419 | +++ xmlobjects.go 2013-07-12 13:07:30 +0000 | |||
420 | @@ -626,7 +626,6 @@ | |||
421 | 626 | } | 626 | } |
422 | 627 | 627 | ||
423 | 628 | type VirtualNetworkSite struct { | 628 | type VirtualNetworkSite struct { |
424 | 629 | XMLName string `xml:"VirtualNetworkSite"` | ||
425 | 630 | Name string `xml:"name,attr"` | 629 | Name string `xml:"name,attr"` |
426 | 631 | AffinityGroup string `xml:"AffinityGroup,attr"` | 630 | AffinityGroup string `xml:"AffinityGroup,attr"` |
427 | 632 | AddressSpacePrefixes []string `xml:"AddressSpace>AddressPrefix"` | 631 | AddressSpacePrefixes []string `xml:"AddressSpace>AddressPrefix"` |
Looks generally good but I've got a question (see [0]).
[0]
76 + for _, existingSite := range *networkConfig. VirtualNetworkS ites {
77 + if existingSite.Name == site.Name {
78 + // Network already defined.
79 + return nil
80 + }
That's a weird behavior don't you think? Say I try adding a network with the same name but a different affinity group; the method will return with a nil error so I'll think that everything is ok and it will be a lie :). I /think/ returning an error (maybe only if the network you're trying to add has exactly the same characteristics as the one with the same name which already exists) is more appropriate here. What do you think?
[1]
102 + for _, existingSite := range *networkConfig. VirtualNetworkS ites { virtualNetworkS ites, existingSite)
103 + if existingSite.Name != siteName {
104 + virtualNetworkSites = append(
105 + }
We cannot just remove the network in question from networkConfig. VirtualNetworkS ites can we :/
[2]
208 + virtualNetwork := &VirtualNetwork Site{Name: MakeRandomVirtu alNetworkName( "test-" )} etworkSite( virtualNetwork)
209 + err := api.AddVirtualN
210 + c.Assert(err, IsNil)
211 + c.Check(record, HasLen, 2)
It's a detail but that's something Jeroen and I have tried to do in tests, especially when the tests are long: put a empty line between the initialization code and the method you're testing, and another after that. This way, we have 3 visually well-separated blocks: initialization, method call, checks.
[3]
You could add a check in example/ management/ run.go to make sure that the created machine effectively has an IP in the virtual network.