Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1937)

Issue 77850043: Added network inclusion/exclusion to acquireNode

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 8 months ago by hduran
Modified:
7 years, 8 months ago
Reviewers:
rvb, mp+211748, natefinch
Visibility:
Public.

Description

Added network inclusion/exclusion to acquireNode I added Networks structure as a parameter of acquireNode in maas provider. Its two members IncludedNetworks and ExcludedNetworks get converted, in a similar fashion than constraints, into url encoded parameters, according to ~maas-maintainers/maas/trunk/view/head:/docs/networks.rst they become, respectively "networks" and "not_networks" For this I create an analog to convertConstraints which I called convertNetworks, mostly to keep the two conceptually separated. I added testing for the four variants I could think of the structure values that where representative. I also made a small cosmetic change to the converConstraints test since it was very hard to read and I needed to based my own test on it https://code.launchpad.net/~hduran-8/juju-core/add_networks_to_maas_acquireNode/+merge/211748 Requires: https://code.launchpad.net/~gz/juju-core/add_startinstance_params/+merge/211588 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 5

Patch Set 2 : Added network inclusion/exclusion to acquireNode #

Patch Set 3 : Added network inclusion/exclusion to acquireNode #

Unified diffs Side-by-side diffs Delta from patch set Stats (+85 lines, -19 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M provider/maas/environ.go View 1 2 2 chunks +20 lines, -2 lines 0 comments Download
M provider/maas/environ_whitebox_test.go View 1 2 4 chunks +63 lines, -17 lines 0 comments Download

Messages

Total messages: 9
hduran
Please take a look.
7 years, 8 months ago (2014-03-19 15:09:13 UTC) #1
natefinch
On 2014/03/19 15:09:13, hduran wrote: > Please take a look. LGTM with one slight change.
7 years, 8 months ago (2014-03-19 15:32:34 UTC) #2
natefinch
Doh, forgot to do the publish and mail thing. Here's the change I mentioned before. ...
7 years, 8 months ago (2014-03-19 15:33:52 UTC) #3
hduran
Please take a look. https://codereview.appspot.com/77850043/diff/1/provider/maas/environ.go File provider/maas/environ.go (right): https://codereview.appspot.com/77850043/diff/1/provider/maas/environ.go#newcode172 provider/maas/environ.go:172: func convertNetworks(params *url.Values, nets environs.Networks) ...
7 years, 8 months ago (2014-03-19 15:48:15 UTC) #4
hduran
I did change the name, it makes more sense now. Also I changed params to ...
7 years, 8 months ago (2014-03-19 15:48:44 UTC) #5
rvb
https://codereview.appspot.com/77850043/diff/1/provider/maas/environ.go File provider/maas/environ.go (right): https://codereview.appspot.com/77850043/diff/1/provider/maas/environ.go#newcode175 provider/maas/environ.go:175: //XXX Is this suposed to be a comma separated ...
7 years, 8 months ago (2014-03-19 15:51:32 UTC) #6
hduran
Please take a look. https://codereview.appspot.com/77850043/diff/1/provider/maas/environ.go File provider/maas/environ.go (right): https://codereview.appspot.com/77850043/diff/1/provider/maas/environ.go#newcode175 provider/maas/environ.go:175: //XXX Is this suposed to ...
7 years, 8 months ago (2014-03-19 16:10:31 UTC) #7
rvb
[...] > Done. Thanks for the changes. LGTM.
7 years, 8 months ago (2014-03-19 16:41:26 UTC) #8
natefinch
7 years, 8 months ago (2014-03-19 18:47:25 UTC) #9
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b