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-723] Added cloud specific parameter to base compute wait_un… #548

Closed
wants to merge 2 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@davidwilson2038
Contributor

davidwilson2038 commented Jul 17, 2015

Fixed LIBCLOUD-723 bug with Azure driver where the wait_until_running method is broken because it calls self.list_nodes() without any options (list_nodes is defined as requiring an ex_cloud_service parameter). I added an optional ex_cloud_opts parameter to the wait_until_running method to make it work and also allow users of other drivers to specify additional configuration parameters if their list_nodes function allows.

[LIBCLOUD-723] Added cloud specific parameter to base compute wait_un…
…til_running method to make compatible with Azure
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 19, 2015

Member

@davidwilson2038 Good catch and thanks.

Member

Kami commented Jul 19, 2015

@davidwilson2038 Good catch and thanks.

Show outdated Hide outdated libcloud/compute/base.py
@@ -1260,8 +1260,9 @@ def delete_key_pair(self, key_pair):
raise NotImplementedError(
'delete_key_pair not implemented for this driver')
def wait_until_running(self, nodes, wait_period=3, timeout=600,
ssh_interface='public_ips', force_ipv4=True):
def wait_until_running(self, nodes, ex_cloud_opts=None, wait_period=3,

This comment has been minimized.

@Kami

Kami Jul 19, 2015

Member

Can you please move this new argument to the end so it's the last argument?

This way the change is backward compatible and doesn't break existing code if using non keyword arguments.

@Kami

Kami Jul 19, 2015

Member

Can you please move this new argument to the end so it's the last argument?

This way the change is backward compatible and doesn't break existing code if using non keyword arguments.

This comment has been minimized.

@Kami

Kami Jul 19, 2015

Member

Please also add a docstring which explains what this argument does.

Edit: Never mind, I see the docstring is already there.

@Kami

Kami Jul 19, 2015

Member

Please also add a docstring which explains what this argument does.

Edit: Never mind, I see the docstring is already there.

Show outdated Hide outdated libcloud/compute/base.py
@@ -1316,7 +1320,7 @@ def filter_addresses(addresses):
uuids = set([node.uuid for node in nodes])
while time.time() < end:
all_nodes = self.list_nodes()
all_nodes = self.list_nodes(ex_cloud_opts)

This comment has been minimized.

@Kami

Kami Jul 19, 2015

Member

It would be better to make this argument a dict - this way it's more generic and user can pass multiple arguments to the list method. And we should also call it ex_list_nodes_kwargs to make it more explicit.

Then we would do something like that:

...
all_nodes = self.list_nodes(**ex_list_nodes_kwargs)
@Kami

Kami Jul 19, 2015

Member

It would be better to make this argument a dict - this way it's more generic and user can pass multiple arguments to the list method. And we should also call it ex_list_nodes_kwargs to make it more explicit.

Then we would do something like that:

...
all_nodes = self.list_nodes(**ex_list_nodes_kwargs)

This comment has been minimized.

@davidwilson2038

davidwilson2038 Jul 19, 2015

Contributor

This is good solution. I have implemented it and edited the docstring to describe the changes.

@davidwilson2038

davidwilson2038 Jul 19, 2015

Contributor

This is good solution. I have implemented it and edited the docstring to describe the changes.

ssh_interface='public_ips', force_ipv4=True):
def wait_until_running(self, nodes, wait_period=3,
timeout=600, ssh_interface='public_ips',
force_ipv4=True, **ex_list_nodes_kwargs):

This comment has been minimized.

@Kami

Kami Aug 7, 2015

Member

There should be no ** here since user should pass in a dict to the function. I will remove it when merging the patch.

@Kami

Kami Aug 7, 2015

Member

There should be no ** here since user should pass in a dict to the function. I will remove it when merging the patch.

@asfgit asfgit closed this in 4cc3adf Aug 7, 2015

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Aug 7, 2015

Member

Made this method signature fix (4cc3adf) and merged changes into trunk.

Thanks.

Member

Kami commented Aug 7, 2015

Made this method signature fix (4cc3adf) and merged changes into trunk.

Thanks.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Aug 7, 2015

Member

@davidwilson2038 On a related note - Now that this is merged into trunk, you can also override wait_until_running method in the Azure driver - you can simply call parent class method with the correct ex_list_nodes_kwargs argument.

Member

Kami commented Aug 7, 2015

@davidwilson2038 On a related note - Now that this is merged into trunk, you can also override wait_until_running method in the Azure driver - you can simply call parent class method with the correct ex_list_nodes_kwargs argument.

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