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

Issue: LIBCLOUD-477 Add list/delete/create network (VPC) calls within EC... #203

Closed
wants to merge 7 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@cderamus
Contributor

cderamus commented Dec 20, 2013

...2

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
params = {'Action': 'DescribeVpcs'}
result = self.connection.request(self.path,
params=params.copy()).object

This comment has been minimized.

@Kami

Kami Dec 22, 2013

Member

Is .copy call necessary here?

request method shouldn't mutate params so the copy call here should be unnecessary.

@Kami

Kami Dec 22, 2013

Member

Is .copy call necessary here?

request method shouldn't mutate params so the copy call here should be unnecessary.

This comment has been minimized.

@cderamus

cderamus Dec 22, 2013

Contributor

Certainly isn't, just came over with a copy paste from a different call :).

@cderamus

cderamus Dec 22, 2013

Contributor

Certainly isn't, just came over with a copy paste from a different call :).

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
Return all private virtual private cloud (VPC) networks
:return: list of network dicts
:rtype: ``list``

This comment has been minimized.

@Kami

Kami Dec 22, 2013

Member

list of dict

@Kami

Kami Dec 22, 2013

Member

list of dict

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
namespace=NAMESPACE)
# Get tags
tags = dict((findtext(element=item,

This comment has been minimized.

@Kami

Kami Dec 22, 2013

Member

Looks like a regular for loop would make this couple of lines a bit more readable.

@Kami

Kami Dec 22, 2013

Member

Looks like a regular for loop would make this couple of lines a bit more readable.

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
# Attempt to tag our network if the name was provided
if name is not None:
# Build a resource object

This comment has been minimized.

@Kami

Kami Dec 22, 2013

Member

If you define a new class which represents a Network, you could get rid of this code.

In cases like this, defining a class is also generally better than using a dict. Dict enforces no structure which means it's fragile and it also makes it harder for the end user to know what to expect.

Then you can also modify ex_list_networks to return a list of instances of this class.

@Kami

Kami Dec 22, 2013

Member

If you define a new class which represents a Network, you could get rid of this code.

In cases like this, defining a class is also generally better than using a dict. Dict enforces no structure which means it's fragile and it also makes it harder for the end user to know what to expect.

Then you can also modify ex_list_networks to return a list of instances of this class.

This comment has been minimized.

@cderamus

cderamus Dec 22, 2013

Contributor

No objection here. I'll go ahead and make that change now.

@cderamus

cderamus Dec 22, 2013

Contributor

No objection here. I'll go ahead and make that change now.

cderamus and others added some commits Dec 22, 2013

Merge pull request #2 from apache/trunk
Pulling in latest updates
Merge pull request #3 from apache/trunk
Pulling in latest changes
@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
result = self.connection.request(self.path, params=params).object
# Get our properties
response = {'vpc_id': findtext(element=result,

This comment has been minimized.

@Kami

Kami Dec 23, 2013

Member

This method should now also return EC2Network instance.

Something like network = self._to_network(elem=respose.object should do (you might need to select a correct element first though).

You can then also pass this instance to self.ex_create_tags method.

@Kami

Kami Dec 23, 2013

Member

This method should now also return EC2Network instance.

Something like network = self._to_network(elem=respose.object should do (you might need to select a correct element first though).

You can then also pass this instance to self.ex_create_tags method.

This comment has been minimized.

@Kami

Kami Dec 23, 2013

Member

I would also appreciate if you could synchronize your branch with trunk after you make those changes.

Otherwise I will have a bad time merging this patch since there will be a bunch of merge conflicts.

@Kami

Kami Dec 23, 2013

Member

I would also appreciate if you could synchronize your branch with trunk after you make those changes.

Otherwise I will have a bad time merging this patch since there will be a bunch of merge conflicts.

This comment has been minimized.

@cderamus

cderamus Dec 24, 2013

Contributor

Yup my fault on that. I'll make sure to synchronize my branch before submitting future PRs.

@cderamus

cderamus Dec 24, 2013

Contributor

Yup my fault on that. I'll make sure to synchronize my branch before submitting future PRs.

This comment has been minimized.

@Kami

Kami Dec 24, 2013

Member

No problem and thanks for all those PRs :)

@Kami

Kami Dec 24, 2013

Member

No problem and thanks for all those PRs :)

Chris DeRamus added some commits Dec 24, 2013

Chris DeRamus
Merge branch 'LIBCLOUD-467_Add_VPC_Lifecycle_Support' into trunk
Conflicts:
	libcloud/compute/drivers/ec2.py
	libcloud/test/compute/test_ec2.py
Chris DeRamus
Updated ex_create_network to return an EC2Network object
Updated test_ex_create_network accordingly
return network
def ex_delete_network(self, vpc_id):

This comment has been minimized.

@Kami

Kami Dec 24, 2013

Member

Now that we have EC2Network class, this method should take an instance of this class to be consistent with other methods (delete_key_pair, destroy_volume, etc.).

If you only have an ID, you can always constructor the EC2Network object manually and pass it to this method to avoid the ex_list_networks call.

@Kami

Kami Dec 24, 2013

Member

Now that we have EC2Network class, this method should take an instance of this class to be consistent with other methods (delete_key_pair, destroy_volume, etc.).

If you only have an ID, you can always constructor the EC2Network object manually and pass it to this method to avoid the ex_list_networks call.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Dec 25, 2013

Member

@cderamus I went ahead, made the "ex_delete_network" change mentioned above and merged changes into trunk.

I had some issues (conflicts) with applying the patch and squashing the commits so I just went ahead and directly merged your branch into trunk.

Member

Kami commented Dec 25, 2013

@cderamus I went ahead, made the "ex_delete_network" change mentioned above and merged changes into trunk.

I had some issues (conflicts) with applying the patch and squashing the commits so I just went ahead and directly merged your branch into trunk.

@cderamus cderamus closed this Dec 25, 2013

@cderamus cderamus deleted the DivvyCloud:LIBCLOUD-467_Add_VPC_Lifecycle_Support branch Dec 27, 2013

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