-
Notifications
You must be signed in to change notification settings - Fork 194
WIP: Add support for specifying a subnet on a private VLAN #923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- When a private subnet id is given in addition to a private
VLAN id this extra information will be passed to the create
request to further specify the location of the VM
|
Current code fails the tox analysis: |
allmightyspiff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add in public_vlan_subnet support as well.
And add in unit test support for these new features.
| 'primaryNetworkComponent': { | ||
| "networkVlan": {"id": int(public_vlan)}}}) | ||
|
|
||
| if private_vlan: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts here are to break out the private/public vlan stuff into a helper function.
if private_vlan or public_vlan:
network_component = self._create_network_components(private_vlan, public_vlan, private_vlan_subnet, public_vlan_subnet)
date.update(network_component)Or something similar would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions, I had just started doing that very thing - however, just to pre-warn you it seems there is a tox check for max number of lines in a module, and we have just crossed the 1000 limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for that one, you can just add it to the ignored list at the top of this module. I don't want to split this file out at the moment, although I will likely do so in the future.
| type=click.INT) | ||
| @click.option('--vlan-private-subnet', | ||
| help="The ID of the private VLAN subnet on which you want the virtual " | ||
| "server placed", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Max line length is 120, so put this all on one line please.
| data['private_vlan'] = args['vlan_private'] | ||
|
|
||
| if args.get('vlan_private_subnet'): | ||
| data['private_vlan_subnet'] = args['vlan_private_subnet'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend naming this private_subnet instead of private_vlan_subnet.
| help="The ID of the private VLAN on which you want the virtual " | ||
| "server placed", | ||
| type=click.INT) | ||
| @click.option('--vlan-private-subnet', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here just, --private-subnet.
| "networkVlan": {"id": int(private_vlan)}}}) | ||
| # As per https://stackoverflow.com/questions/37592080/create-a-softlayer-virtual-guest-on-a-specific-subnet | ||
| # we can specify a subnet under the VLAN | ||
| if private_vlan_subnet: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be inside the private_vlan if statement since I don't think the API requires a vlan id when supplying a subnet id.
|
Closing this as it was implemented as part of #995 , and that seems to be working well for me now 👍 |
VLAN id this extra information will be passed to the create
request to further specify the location of the VM