Skip to content

Conversation

@FernandoOjeda
Copy link
Contributor

New Feature to create a vm in a specific subnet: #921

@coveralls
Copy link

coveralls commented Jul 5, 2018

Coverage Status

Coverage increased (+0.3%) to 88.787% when pulling 488979b on FernandoOjeda:fo_vlan_subnet into 6332ffe on softlayer:master.

Copy link
Member

@allmightyspiff allmightyspiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, add a test for _create_network_components in managers/vs.py to test all the if cases

# Get the SSH keys
if args.get('key'):
keys = []
for key in args.get('key'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there any reason for moving these lines to their own function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem was that it exceeded the maximum number of branches in the method and I only made a refactor of the for cycle in another method using the IDE's refactor options.

return data


def _add_keys(args, client, keys):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like functions that modify the data that gets passed in. this function should return keys instead of modifying the array that gets passed in.


return data

def _create_network_components(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try something like this instead.

parameters = {}
if private_vlan:
    parameters['primaryBackendNetworkComponent'] = { "networkVlan": {"id": int(private_vlan)}}
if public_vlan:
   parameters['primaryNetworkComponent'] =  { "networkVlan": {"id": int(public_vlan)}}
if public_subnet:
   if public_vlan is None:
       throw Excetions("You need to specify a public_vlan with public_subnet") 
   parameters['primaryNetworkComponent']['networkVlan]['primarySubnet'] = {'id' : int(public_subnet)}
if private_subnet:
   if private_vlan is None:
       throw Excetions("You need to specify a private_vlan with private_subnet") 
   parameters['primaryBackendNetworkComponent']['networkVlan]['primarySubnet'] = {'id' : int(private_subnet)}


if args.get('subnet_public'):
data['public_subnet'] = args['subnet_public']
data['public_subnet'] = args.get('subnet_public', None)
Copy link
Contributor Author

@FernandoOjeda FernandoOjeda Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to implement the switcher option for this method, but we get the ¨args¨ parameter, it's not a specific data. I was implementing like this:

switcher = {
    args.get('san'): attribute_san(args, data),
    args.get('os'): attribute_os(args, data),
    args.get('image'): attribute_image(args, client, data),
    args.get('datacenter'): attribute_datacenter(args, data),
    args.get('network'): attribute_network(args, data),
    args.get('postinstall'): attribute_postinstall(args, data),
    args.get('key'): attribute_key(args, client, data),
    args.get('vlan_public'): attribute_vlan_public(args, data),
    args.get('vlan_private'): attribute_vlan_private(args, data),
    args.get('subnet_public'): attribute_subnet_public(args, data),
    args.get('subnet_private'): attribute_subnet_private(args, data),
    args.get('public_security_group'): attribute_public_security_group(args, data),
    args.get('private_security_group'): attribute_private_security_group(args, data),
    args.get('tag'): attribute_tag(args, data),
    args.get('host_id'): attribute_host_id(args, data),
}

return data

def attribute_host_id(args, data):
data['host_id'] = args['host_id']

def attribute_tag(args, data):
data['tags'] = ','.join(args['tag'])

def attribute_private_security_group(args, data):
priv_groups = args.get('private_security_group')
data['private_security_groups'] = [group for group in priv_groups]
..........................

In this case the attribute switches will not be use, because in each method each data from args will be added to the data attribute.
That's why I used the first option you suggested.

Copy link
Member

@allmightyspiff allmightyspiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I merge this, can you test to make sure it actually provisions a VSI on the correct public and private subnets.

Thanks

@FernandoOjeda
Copy link
Contributor Author

Yes, it is working well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants