Fixes the function ex_create_floating_ip for Openstack#725
Conversation
|
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. |
…ng_ip. This is useful in case there is only one IP pool available.
|
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. |
|
One more note, I don't have an option to test my code against an Openstack tenant with only one floating IP pool. |
| """ | ||
| if (ip_pool is None) and (len(self.ex_list_floating_ip_pools()) == 1): | ||
| ip_pool = self.ex_list_floating_ip_pools()[0].name | ||
| elif (ip_pool is None) and (len(self.ex_list_floating_ip_pools()) > 1): |
There was a problem hiding this comment.
I'm not sure about this, if the behavior of open-stack changes, we've bound ourselves to this assumption. I would remove this safety check and instead say
data = {} if ip_pool is None else {'pool': ip_pool}keeping it all on one line.
There was a problem hiding this comment.
Looks nicer indeed, but leaving data = {} raises an exception.
Would you care to explain what assumption are you talking about? I guess you are talking about selecting the first IP pool in case the user did not provide it as an attribute. I did it like this, because data = {} does not work with Openstack implementation, but it may work in cases where there is only one pool available (I can't test that). The problem with your suggestion is that it raises an exception in case the user did not provide the pool name and the Openstack infrastructure has more than one pool. As I see it, it is safer to check for the number of pools available in case the user did not provide us with the pool name. I will update my code shortly ...
There was a problem hiding this comment.
The assumption that it raises an exception, I'm saying if OpenStack change that behavior, the users have to upgrade libcloud to get the benefit.
If you update the docstring instead to say it is required under what circumstances
|
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
|
|
perfect. I think this behavior is correct since the error states what the issue is. Merging |
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.