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

adds ex_security_group_ids to ec2 in order to be able to launch nodes with security groups on a VPC #373

Closed
wants to merge 5 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@Itxaka
Contributor

Itxaka commented Oct 11, 2014

Due to amazon having a different param for the security groups on an instance launched on a VPC and libcloud already supports VPCs (ex_subnet) this is the piece is missing to support the security groups on a VPC node

  • adds a ex_security_group_ids kwarg which is a list of security group ids
  • checks for the existence of ex_subnet if ex_security_group_ids is called, as it depends on it to make sense
  • adds a test to see if the params are being constructed correctly

EC2 API page: http://docs.aws.amazon.com/AWSEC2/latest/APIReference/ApiReference-query-RunInstances.html

SecurityGroupId.n
One or more security group IDs. You can create a security group using CreateSecurityGroup.
Type: String
Default: Amazon EC2 uses the default security group.
Required: No
SecurityGroup.n
[EC2-Classic, default VPC] One or more security group names. For a nondefault VPC, you must use SecurityGroupId.n.
Type: String
Default: Amazon EC2 uses the default security group.
Required: No
@Itxaka

This comment has been minimized.

Show comment
Hide comment
@Itxaka

Itxaka Oct 11, 2014

Contributor

@Kami :) :shipit:

Contributor

Itxaka commented Oct 11, 2014

@Kami :) :shipit:

@Kami

View changes

Show outdated Hide outdated libcloud/compute/drivers/ec2.py
raise ValueError('You can only supply ex_security_group_id'
' combinated with ex_subnet')
security_group_id = kwargs.get('ex_security_group_ids', None)

This comment has been minimized.

@Kami

Kami Oct 19, 2014

Member

This variable stores a list so it should be called security_group_ids.

@Kami

Kami Oct 19, 2014

Member

This variable stores a list so it should be called security_group_ids.

@Kami

View changes

Show outdated Hide outdated libcloud/test/compute/test_ec2.py
@@ -871,6 +872,22 @@ def test_create_node_ex_security_groups(self):
ex_securitygroup=security_groups,
ex_security_groups=security_groups)
def test_create_node_ex_security_group_id(self):

This comment has been minimized.

@Kami

Kami Oct 19, 2014

Member

Also needs a test case for edge case (when user specifies ex_security_group_ids, but not ex_subnet).

@Kami

Kami Oct 19, 2014

Member

Also needs a test case for edge case (when user specifies ex_security_group_ids, but not ex_subnet).

This comment has been minimized.

@Itxaka

Itxaka Oct 20, 2014

Contributor

I tried but could not catch the Exception with self.assertRaises, it actually raised the Exception. Will look into this.

@Itxaka

Itxaka Oct 20, 2014

Contributor

I tried but could not catch the Exception with self.assertRaises, it actually raised the Exception. Will look into this.

This comment has been minimized.

@Kami

Kami Oct 21, 2014

Member

That's probably because you were actually calling the method yourself and not just passing a callable to assertRaises (you should only pass callable + arguments to assertRaises and let assetRaises call the callable).

For example:

self.assertRaises(ValueError, myfunc(blah, foo=bar))

Not good because you are calling the function yourself.

For example:

self.assertRaises(ValueError, myfunc, blah, foo=bar)

Good - you are letting assertRaises call the function and intercept the exceptions.

@Kami

Kami Oct 21, 2014

Member

That's probably because you were actually calling the method yourself and not just passing a callable to assertRaises (you should only pass callable + arguments to assertRaises and let assetRaises call the callable).

For example:

self.assertRaises(ValueError, myfunc(blah, foo=bar))

Not good because you are calling the function yourself.

For example:

self.assertRaises(ValueError, myfunc, blah, foo=bar)

Good - you are letting assertRaises call the function and intercept the exceptions.

@@ -2135,6 +2135,10 @@ def create_node(self, **kwargs):
assign to the node.
:type ex_security_groups: ``list``
:keyword ex_security_group_ids: A list of ids of security groups to

This comment has been minimized.

@Kami

Kami Oct 19, 2014

Member

Edit: Ignore this comment - pr description already addresses it.

It's just unfortunate that existing argument is called ex_security_groups instead of ex_security_group_names.

@Kami

Kami Oct 19, 2014

Member

Edit: Ignore this comment - pr description already addresses it.

It's just unfortunate that existing argument is called ex_security_groups instead of ex_security_group_names.

@Itxaka

This comment has been minimized.

Show comment
Hide comment
@Itxaka

Itxaka Nov 19, 2014

Contributor

@Kami ?

Contributor

Itxaka commented Nov 19, 2014

@Kami ?

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Nov 19, 2014

Member

@Itxaka Sorry for the delay.

I will have a look later today and hopefully also get it merged then.

Member

Kami commented Nov 19, 2014

@Itxaka Sorry for the delay.

I will have a look later today and hopefully also get it merged then.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Nov 19, 2014

Member

LGTM. Patch has been merged into trunk.

Thanks.

Member

Kami commented Nov 19, 2014

LGTM. Patch has been merged into trunk.

Thanks.

@asfgit asfgit closed this in d7c8671 Nov 19, 2014

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