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-466 Add ingress/egress VPC support to security group rul... #202

Closed
wants to merge 2 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@cderamus
Contributor

cderamus commented Dec 20, 2013

...es

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
if cidr_ips is not None:
i = 1
ip_ranges = {}
for cidr_ip in cidr_ips:

This comment has been minimized.

@Kami

Kami Dec 20, 2013

Member

You could get rid of two lines if you use enumerate which is considered more "Pythonic".

So something like:

for index, cidr_ip in enumerate(cidr_ips, 1)
...
@Kami

Kami Dec 20, 2013

Member

You could get rid of two lines if you use enumerate which is considered more "Pythonic".

So something like:

for index, cidr_ip in enumerate(cidr_ips, 1)
...

This comment has been minimized.

@cderamus

cderamus Dec 20, 2013

Contributor

Nice call. I will make that change momentarily.

@cderamus

cderamus Dec 20, 2013

Contributor

Nice call. I will make that change momentarily.

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
:param cidr_ips: The list of ip ranges to allow traffic for.
:type cidr_ips: ``list``
:param group_pairs: User/group pairs to allow traffic for.

This comment has been minimized.

@Kami

Kami Dec 20, 2013

Member

It would be nice to include an example of couple of possible values for this argument since it's not immediately obvious after just reading the docstrings.

@Kami

Kami Dec 20, 2013

Member

It would be nice to include an example of couple of possible values for this argument since it's not immediately obvious after just reading the docstrings.

This comment has been minimized.

@Kami

Kami Dec 20, 2013

Member

Also, seems like the type should be list of dict and not dict

@Kami

Kami Dec 20, 2013

Member

Also, seems like the type should be list of dict and not dict

This comment has been minimized.

@cderamus

cderamus Dec 20, 2013

Contributor

Noted!

@cderamus

cderamus Dec 20, 2013

Contributor

Noted!

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
'IpPermissions.1.FromPort': from_port,
'IpPermissions.1.ToPort': to_port}
if cidr_ips is not None:

This comment has been minimized.

@Kami

Kami Dec 20, 2013

Member

There seems to be a lot of duplicated code between the methods.

To avoid that, it would be great if you could split duplicated code which populates the params dict into a separate method. You could call it _get_common_security_group_params or something like that.

Then inside those methods, you could do something like:

params = self._get_common_params...
params['Action'] = 'Action'
@Kami

Kami Dec 20, 2013

Member

There seems to be a lot of duplicated code between the methods.

To avoid that, it would be great if you could split duplicated code which populates the params dict into a separate method. You could call it _get_common_security_group_params or something like that.

Then inside those methods, you could do something like:

params = self._get_common_params...
params['Action'] = 'Action'
Chris DeRamus
Getting rid of duplicate code throughout the four calls.
Added additional documentation for user/group pairs.
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Dec 21, 2013

Member

While merging and testing the patch, I have noticed a test failure under Python 2.5. I have forgot that enumerate function in 2.5 doesn't support start argument.

I've fixed that, squashed the commits and merged patch into trunk.

Thanks!

Member

Kami commented Dec 21, 2013

While merging and testing the patch, I have noticed a test failure under Python 2.5. I have forgot that enumerate function in 2.5 doesn't support start argument.

I've fixed that, squashed the commits and merged patch into trunk.

Thanks!

@cderamus cderamus closed this Dec 22, 2013

@cderamus cderamus deleted the DivvyCloud:LIBCLOUD-466_Add_VPC_Ingress_Egress_Rules branch Dec 22, 2013

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