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-464: Add security group delete/destroy into the EC2 drive... #199

Closed
wants to merge 3 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@cderamus
Contributor

cderamus commented Dec 20, 2013

...r

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
@@ -1277,6 +1277,31 @@ def ex_create_security_group(self, name, description):
'GroupDescription': description}
return self.connection.request(self.path, params=params).object
def ex_destroy_security_group(self, group_id=None, group_name=None):

This comment has been minimized.

@Kami

Kami Dec 20, 2013

Member

For consistency with other drivers and to make it easier to promote security group management methods to a top-level API in the near future, I would do the following:

  1. Add two new methods - ex_destroy_security_group_by_id(group_id) and ex_destroy_security_group_by_name(group_name)
  2. Add a wrapper method - ex_destroy_security_group(name) which calls ex_destroy_security_group_by_name underneath

As noted above, those methods will hopefully be promoted to a top-level API in the near-future and guaranteeing consistency and compatibility between drivers will be a bit easier then.

Edit: Fixed a typo.

@Kami

Kami Dec 20, 2013

Member

For consistency with other drivers and to make it easier to promote security group management methods to a top-level API in the near future, I would do the following:

  1. Add two new methods - ex_destroy_security_group_by_id(group_id) and ex_destroy_security_group_by_name(group_name)
  2. Add a wrapper method - ex_destroy_security_group(name) which calls ex_destroy_security_group_by_name underneath

As noted above, those methods will hopefully be promoted to a top-level API in the near-future and guaranteeing consistency and compatibility between drivers will be a bit easier then.

Edit: Fixed a typo.

@cderamus cderamus closed this Dec 20, 2013

@cderamus cderamus deleted the DivvyCloud:LIBCLOUD-464_Add_Delete_Security_Group branch Dec 20, 2013

@cderamus cderamus restored the DivvyCloud:LIBCLOUD-464_Add_Delete_Security_Group branch Dec 20, 2013

@cderamus cderamus reopened this Dec 20, 2013

@cderamus

This comment has been minimized.

Show comment
Hide comment
@cderamus

cderamus Dec 20, 2013

Contributor

Let me know if this is more in the vein of what you'd like buddy.

Contributor

cderamus commented Dec 20, 2013

Let me know if this is more in the vein of what you'd like buddy.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Dec 21, 2013

Member

Ugh, sorry, looks like I've missed one thing in my previous review.

For consistency with other drivers, can you please also rename the methods from "destroy" to "delete" (ex_destroy_security_group -> ex_delete_security_group, ...) ?

Besides that, the patch looks good.

Member

Kami commented Dec 21, 2013

Ugh, sorry, looks like I've missed one thing in my previous review.

For consistency with other drivers, can you please also rename the methods from "destroy" to "delete" (ex_destroy_security_group -> ex_delete_security_group, ...) ?

Besides that, the patch looks good.

@cderamus

This comment has been minimized.

Show comment
Hide comment
@cderamus

cderamus Dec 21, 2013

Contributor

Ah, nice catch on that. I try to stay consistent, but I clearly missed that one. I've updated for your review!

Contributor

cderamus commented Dec 21, 2013

Ah, nice catch on that. I try to stay consistent, but I clearly missed that one. I've updated for your review!

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Dec 22, 2013

Member

Thanks!

While reviewing and merging the patch, I've noticed that you forgot to update ex_delete_security_group method and the tests didn't detect that because they had a typo in the name so they didn't actually run (text instead of test).

I've squashed your commits, fixed those two issues in 4df8dd2 & adba6de and merged your patch into trunk.

When I find some more time, I'll also have a look at how we can automatically detect and prevent issues like this from happening in the feature. It seems that for a start, we could do a simple integration with coverage.py and make sure that all the newly added test cases are actually executed.

Member

Kami commented Dec 22, 2013

Thanks!

While reviewing and merging the patch, I've noticed that you forgot to update ex_delete_security_group method and the tests didn't detect that because they had a typo in the name so they didn't actually run (text instead of test).

I've squashed your commits, fixed those two issues in 4df8dd2 & adba6de and merged your patch into trunk.

When I find some more time, I'll also have a look at how we can automatically detect and prevent issues like this from happening in the feature. It seems that for a start, we could do a simple integration with coverage.py and make sure that all the newly added test cases are actually executed.

@cderamus

This comment has been minimized.

Show comment
Hide comment
@cderamus

cderamus Dec 22, 2013

Contributor

Ah, that's what I get for updating code with egg nog in hand. Thanks for catching that!

Contributor

cderamus commented Dec 22, 2013

Ah, that's what I get for updating code with egg nog in hand. Thanks for catching that!

@cderamus cderamus closed this Dec 22, 2013

@cderamus cderamus deleted the DivvyCloud:LIBCLOUD-464_Add_Delete_Security_Group branch Dec 22, 2013

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