Skip to content
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

OpenStack public ip addresses fix #235

Closed
wants to merge 1 commit into from

Conversation

@mgogoulos
Copy link
Contributor

mgogoulos commented Jan 31, 2014

No description provided.

@mgogoulos
Copy link
Contributor Author

mgogoulos commented Jan 31, 2014

private_ips.append(ip)
else:
public_ips.append(ip)
except:

This comment has been minimized.

@Kami

Kami Jan 31, 2014 Member

It would be good to catch a more specific exception.

@@ -1910,7 +1911,15 @@ def _to_node(self, api_node):
if label in public_networks_labels:
public_ips.extend(ips)
else:
private_ips.extend(ips)
for ip in ips:

This comment has been minimized.

@Kami

Kami Jan 31, 2014 Member

It needs a test case which tests the following scenarios:

  • Public IPv4
  • Public IPv6
  • Private IPv4
  • Private IPv6
@Kami
Copy link
Member

Kami commented Feb 11, 2014

@mgogoulos Bump^^

@mgogoulos
Copy link
Contributor Author

mgogoulos commented Feb 28, 2014

I did some changes and pushed the tests (they've already been in test_list_nodes inside).
On my dev environment they pass, but Travis CI fails! Not sure why this happens, self.assertEqual('12.16.18.28', node.public_ips[0]) could easily be written as self.assertTrue('12.16.18.28' in node.public_ips) since the order of the ips has changed and seems to break the tests, what do you think?

@Kami
Copy link
Member

Kami commented Feb 28, 2014

@mgogoulos Yeah, tests shouldn't rely on some arbitrary ordering, but in this case it seems like the order should always be the same? Or am I missing something?

@Kami
Copy link
Member

Kami commented Feb 28, 2014

@mgogoulos In any case, I'm also fine with using in approach.

@mgogoulos
Copy link
Contributor Author

mgogoulos commented Mar 10, 2014

Fixed the tests, ready to merge as I see this issue is referenced on other tickets as well (https://issues.apache.org/jira/browse/LIBCLOUD-452)

@Kami
Copy link
Member

Kami commented Mar 11, 2014

@mgogoulos Thanks.

Can you please sync this branch with trunk and squash all the commits? Currently, I can't apply patch cleanly onto trunk.

Markos Gogoulos
@mgogoulos
Copy link
Contributor Author

mgogoulos commented Mar 11, 2014

branch synced, commits were squashed and patch has been attached to https://issues.apache.org/jira/browse/LIBCLOUD-503

@Kami
Copy link
Member

Kami commented Mar 11, 2014

Merged into trunk.

Thanks.

@asfgit asfgit closed this in 859a0a4 Mar 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.