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

[LIBCLOUD-749] CloudStack: fixed method ex_authorize_security_group_ingress #580

Closed
wants to merge 3 commits into
base: trunk
from

Conversation

Projects
None yet
3 participants
@schaubl
Contributor

schaubl commented Sep 15, 2015

Corresponding ticket in the JIRA issue tracker: LIBCLOUD-749

This PR fixes the following bugs:

  • The docstring doesn't match parameters of the method.
  • Only ICMP and TCP is allowed even if the doc of the parameter protocol mention TCP and UDP.
  • Moreover ICMP cannot be used because there is no way to specify the two required parameters icmptype and icmpcode.
  • The parameter endport is not used if it's defined.
  • The return type is not a list but a dict.
  • The unit test doesn't use parameters in the correct order.

This PR also does the following changes:

  • Improve the docstring to be a bit more detailed (the previous one comes from the official Apache CloudStack documentation).
  • Add a kwargs to allow to provide extra parameters defined in the API and in the docstring.
  • Improve the unit test.
  • Add support for other protocols like AH, GRE and ESP which are available on some Cloud like Exoscale.
def ex_authorize_security_group_ingress(self, securitygroupname, protocol,
cidrlist, startport=None,
endport=None, icmptype=None,
icmpcode=None, **kwargs):

This comment has been minimized.

@Kami

Kami Sep 20, 2015

Member

Can you please explicitly declare all the supported arguments instead of using **kwargs?

@Kami

Kami Sep 20, 2015

Member

Can you please explicitly declare all the supported arguments instead of using **kwargs?

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Sep 20, 2015

Member

Added one comment, besides that, LGTM. Thanks.

/cc @runseb

Member

Kami commented Sep 20, 2015

Added one comment, besides that, LGTM. Thanks.

/cc @runseb

@sebgoa

This comment has been minimized.

Show comment
Hide comment
@sebgoa

sebgoa Sep 21, 2015

Member

LGTM, I will merge in the coming days

Member

sebgoa commented Sep 21, 2015

LGTM, I will merge in the coming days

@schaubl

This comment has been minimized.

Show comment
Hide comment
@schaubl

schaubl Sep 21, 2015

Contributor

@Kami : Thanks for the comment which allowed me to discover a small issue in the docstring (securitygroupid and usersecuritygrouplist were noted as :param instead of :keyword).

Can you please explicitly declare all the supported arguments instead of using **kwargs?

Yes, of course. I used kwargs because it's the way some "unusual" parameters (like domainid and projectid) are passed with this driver. For example the method create_key_pair takes the name of the keypair and then parameters like domainid and projectid are passed via kwargs.

To be consistent I would suggest to keep it like this.
Please tell me if you still want than these parameters to be explicitly declared.

Contributor

schaubl commented Sep 21, 2015

@Kami : Thanks for the comment which allowed me to discover a small issue in the docstring (securitygroupid and usersecuritygrouplist were noted as :param instead of :keyword).

Can you please explicitly declare all the supported arguments instead of using **kwargs?

Yes, of course. I used kwargs because it's the way some "unusual" parameters (like domainid and projectid) are passed with this driver. For example the method create_key_pair takes the name of the keypair and then parameters like domainid and projectid are passed via kwargs.

To be consistent I would suggest to keep it like this.
Please tell me if you still want than these parameters to be explicitly declared.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Oct 11, 2015

Member

Sorry for the delay - I went ahead and merged the changes.

For now, lets keep kwargs, but in the future it would be great if we can get rid of it and explicitly declare all the supported arguments - (ab)using kwargs makes many things such as "static" analysis hard / impossible.

Member

Kami commented Oct 11, 2015

Sorry for the delay - I went ahead and merged the changes.

For now, lets keep kwargs, but in the future it would be great if we can get rid of it and explicitly declare all the supported arguments - (ab)using kwargs makes many things such as "static" analysis hard / impossible.

@asfgit asfgit closed this in 550dcf4 Oct 11, 2015

asfgit pushed a commit that referenced this pull request Oct 11, 2015

[LIBCLOUD-749] CloudStack: minor fixes to comply with pep8.
Closes #580

Signed-off-by: Tomaz Muraus <tomaz@apache.org>

asfgit pushed a commit that referenced this pull request Oct 11, 2015

[LIBCLOUD-749] Fixed a minor issue with the docstring of the method e…
…x_authorize_security_group_ingress

Closes #580

Signed-off-by: Tomaz Muraus <tomaz@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment