Skip to content
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

Cloudstack Driver - Create/Delete Network functionality + tests #316

Closed
wants to merge 10 commits into from

Conversation

@boul
Copy link
Contributor

@boul boul commented Jun 11, 2014

Functions added on the cloudstack driver:

ex_list_network_offering
ex_create_network
ex_delete_network

New Object:
CloudStackNetworkOffering

And some additions to the CloudStackNetwork dictionary
netmask
gatway
vpcid

@Kami
Copy link
Member

@Kami Kami commented Jun 11, 2014

/cc @Runseb

Class representing a CloudStack Network Offering.
"""

def __init__(self, name, displaytext, guestiptype, id, serviceofferingid,

This comment has been minimized.

@Kami

Kami Jun 11, 2014
Member

For consistency with existing code and other drivers, please use _ to separate words in the argument names - display_text, guest_ip_type, service_offerring_id, for_vpc, etc.


return networkofferings

def ex_create_network(self, displaytext, name, networkoffering,

This comment has been minimized.

@Kami

Kami Jun 11, 2014
Member

Same here, please use underscore in the argument names.

This comment has been minimized.

@Kami

Kami Jun 11, 2014
Member

Instead of using kwargs, please explicitly declare gateway, netmask and vpc_id argument in the method signature.

Using kwargs in cases like this makes programmatic introspection and many other things hard / impossible - https://libcloud.readthedocs.org/en/latest/development.html#don-t-abuse-kwargs

This comment has been minimized.

@sebgoa

sebgoa Jun 11, 2014
Member

Could you also add deleteNetworkOffering and createNetworkOffering ? Since you are defining the CloudStackNetworkOffering class, you could add the other methods

This comment has been minimized.

@boul

boul Jun 11, 2014
Author Contributor

createNetworkOffering and deleteNetworkOffering are only-available to ROOT-admins. I might add this at a later stage. Especially createNetworkOffering could be tricky based on the different network providers. I'd rather put some focus on CRUD VPC's.

This comment has been minimized.

@Kami

Kami Jun 11, 2014
Member

Yeah, and it's also worth noting that compute drivers should only implement user / non-admin functionality.

We need to find a better and unified solution (maybe a separate driver / class) for handling admin actions, but this requires a bit more thought and work.

This comment has been minimized.

@sebgoa

sebgoa Jun 11, 2014
Member

On Jun 11, 2014, at 3:56 PM, Tomaz Muraus notifications@github.com wrote:

In libcloud/compute/drivers/cloudstack.py:

  •    networkofferings = []
    
  •    for netoffer in netoffers:
    
  •        networkofferings.append(CloudStackNetworkOffering(
    
  •                        netoffer['name'],
    
  •                        netoffer['displaytext'],
    
  •                        netoffer['guestiptype'],
    
  •                        netoffer['id'],
    
  •                        netoffer['serviceofferingid'],
    
  •                        netoffer['forvpc'],
    
  •                        self))
    
  •    return networkofferings
    
  • def ex_create_network(self, displaytext, name, networkoffering,

Yeah, and it's also worth noting that compute drivers should only implement user / non-admin functionality.

We need to find a better and unified solution (maybe a separate driver / class) for handling admin actions, but this requires a bit more thought and work.

Yes agreed, good point by Roland.


Reply to this email directly or view it on GitHub.


return network

def ex_delete_network(self, network, forced=None):

This comment has been minimized.

@Kami

Kami Jun 11, 2014
Member

For consistency with existing code base, please call the argument force instead of forced.

boul added 2 commits Jun 11, 2014
@boul
Copy link
Contributor Author

@boul boul commented Jun 11, 2014

Thx for the quick feedback, see 50e6c75 with the suggested changes for consistancy reasons.

return networkofferings

def ex_create_network(self, displaytext, name, networkoffering,
location, **kwargs):

This comment has been minimized.

@sebgoa

sebgoa Jun 11, 2014
Member

You still need to fix the kwargs issue. We prefer to explicitly list the arguments that you need: gateway, net mask, vpc_id

This comment has been minimized.

@boul

boul Jun 12, 2014
Author Contributor

Fixed it. Cheers!

boul added 3 commits Jun 12, 2014
add optinal args for ex_create_network
extend network dictionary
fix tests accordingly
@sebgoa
Copy link
Member

@sebgoa sebgoa commented Jun 12, 2014

Can you squash your commits ? that would make a clean PR. thx

@asfgit asfgit closed this in 33ca3b2 Jun 12, 2014
@Kami
Copy link
Member

@Kami Kami commented Jun 12, 2014

I've squashed the commits, renamed some variables to use underscores and merged patch into trunk. Thanks.

@boul
Copy link
Contributor Author

@boul boul commented Jun 12, 2014

Thx just squashed before noticing you already did. Thx!

Kind Regards,
Roeland Kuipers

http://www.schubergphilis.com

Sent from a mobile device.

Op 12 jun. 2014 om 20:51 heeft Tomaz Muraus notifications@github.com het volgende geschreven:

I've squashed the commits, renamed some variables to use underscores and merged patch into trunk. Thanks.


Reply to this email directly or view it on GitHub.

@sebgoa
Copy link
Member

@sebgoa sebgoa commented Jun 12, 2014

@Kami you were not that nice with me when I submitted pr :) …maybe once

@Kami
Copy link
Member

@Kami Kami commented Jun 12, 2014

@Runseb heh, I can't always easily squash the commits - if the branch is not in sync with trunk, trying to apply the patch results in conflicts and resolving this is quite painful (although, I do it every now and then).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.