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

Fixes the function ex_create_floating_ip for Openstack #725

Closed
wants to merge 3 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@marko-p

marko-p commented Mar 21, 2016

I'm not sure if this function is supported or not (floating_ip_pool.create_floating_ip is working), but I fixed it since it is the only documented function for creating floating IPs on http://libcloud.readthedocs.org/en/latest/compute/drivers/openstack.html ... and I fixed it before I found the "floating_ip_pool.create_floating_ip" function.

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Mar 21, 2016

Contributor

hi @marko-p thanks for the contribution. I was just looking at the docs and it says that pool is optional http://developer.openstack.org/api-ref-compute-v2.1.html#createFloatingIP please can you extend the logic to support pool as an optional argument, this will also support existing users of the method.

Contributor

tonybaloney commented Mar 21, 2016

hi @marko-p thanks for the contribution. I was just looking at the docs and it says that pool is optional http://developer.openstack.org/api-ref-compute-v2.1.html#createFloatingIP please can you extend the logic to support pool as an optional argument, this will also support existing users of the method.

marko_praprotnik
Makes the ip_pool attribute optional in the function ex_create_floati…
…ng_ip. This is useful in case there is only one IP pool available.
@marko-p

This comment has been minimized.

Show comment
Hide comment
@marko-p

marko-p Mar 21, 2016

Thanks for such a quick reply! The pool is optional only if there is only one pool available. I made the ip_pool attribute optional, but in case there are more than one IP pools available, not providing the ip_pool will return None - without printing anything. I'm not sure this is the correct way to handle this, because I did not read the whole guideline for contributing to libcloud (sorry!), I just looked up other functions in the same file.

marko-p commented Mar 21, 2016

Thanks for such a quick reply! The pool is optional only if there is only one pool available. I made the ip_pool attribute optional, but in case there are more than one IP pools available, not providing the ip_pool will return None - without printing anything. I'm not sure this is the correct way to handle this, because I did not read the whole guideline for contributing to libcloud (sorry!), I just looked up other functions in the same file.

@marko-p

This comment has been minimized.

Show comment
Hide comment
@marko-p

marko-p Mar 21, 2016

One more note, I don't have an option to test my code against an Openstack tenant with only one floating IP pool.

marko-p commented Mar 21, 2016

One more note, I don't have an option to test my code against an Openstack tenant with only one floating IP pool.

@marko-p

This comment has been minimized.

Show comment
Hide comment
@marko-p

marko-p Mar 22, 2016

I modified your suggestion a bit in order to follow the guideline to use "is not None" when checking if a variable is provided or defined

Again, this code raises an exception in case the user does not provide the pool name and there are more than one pools available. Is that ok? Forget that, I've read your answer about what assumption you are talking about :)

marko-p commented Mar 22, 2016

I modified your suggestion a bit in order to follow the guideline to use "is not None" when checking if a variable is provided or defined

Again, this code raises an exception in case the user does not provide the pool name and there are more than one pools available. Is that ok? Forget that, I've read your answer about what assumption you are talking about :)

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Mar 22, 2016

Contributor

perfect. I think this behavior is correct since the error states what the issue is. Merging

Contributor

tonybaloney commented Mar 22, 2016

perfect. I think this behavior is correct since the error states what the issue is. Merging

@asfgit asfgit closed this in ac4a8c1 Mar 22, 2016

asfgit pushed a commit that referenced this pull request Mar 22, 2016

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