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

Update network check to fix missing flags on Windows #404

Merged
merged 8 commits into from
Nov 20, 2019

Conversation

claireyywang
Copy link
Contributor

Warn about missing flags on windows ipconfig output. Closes #403 - false negative warnings on windows.

claireyywang added 4 commits November 19, 2019 11:56
Signed-off-by: claireyywang <clairewang@openrobotics.org>
Signed-off-by: claireyywang <clairewang@openrobotics.org>
Signed-off-by: claireyywang <clairewang@openrobotics.org>
Signed-off-by: claireyywang <clairewang@openrobotics.org>
@claireyywang claireyywang added the in review Waiting for review (Kanban column) label Nov 19, 2019
@claireyywang claireyywang self-assigned this Nov 19, 2019
Signed-off-by: claireyywang <clairewang@openrobotics.org>
@claireyywang
Copy link
Contributor Author

@ros-pull-request-builder retest this please

@claireyywang
Copy link
Contributor Author

Linux Build Status
Linux-aarch64 Build Status
OSX Build Status
Windows Build Status

@@ -66,6 +66,11 @@ def check(self):
return result

has_loopback, has_non_loopback, has_multicast = _check_network_config_helper(ifcfg_ifaces)
if not has_loopback and not has_non_loopback:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this warning only happen on windows since it would be an actual error on other platforms if both of these were False. One way is to make this line check if the platform is windows. Another way would be to make _check_network_config_helper() return None for these values instead of True or False when there's no information, and then check if they're None here.

@@ -66,6 +66,11 @@ def check(self):
return result

has_loopback, has_non_loopback, has_multicast = _check_network_config_helper(ifcfg_ifaces)
if not has_loopback and not has_non_loopback:
# no flags found, otherwise one of them should be True.
result.add_warning('No flags found. \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it should be a warning in the result since there's nothing a user can do to resolve it. Maybe this should be printed, or maybe result should have an even softer category like result.add_note()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this scenario is rare (or I would like to keep it rare) enough to use a print statement instead of extending the result category, but good point and I will modify it.

Signed-off-by: claireyywang <clairewang@openrobotics.org>
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!


Why _check_network_config_helper doesn't use ifcfg_ifaces?
Shouldn't line 39 be:

    for _, iface in ifcfg_ifaces.items():

?

@claireyywang
Copy link
Contributor Author

LGTM!

Why _check_network_config_helper doesn't use ifcfg_ifaces?
Shouldn't line 39 be:

    for _, iface in ifcfg_ifaces.items():

?

Good catch!

Signed-off-by: claireyywang <clairewang@openrobotics.org>
@@ -36,7 +36,7 @@ def _is_unix_like_platform() -> bool:
def _check_network_config_helper(ifcfg_ifaces: dict) -> Tuple[bool, bool, bool]:
"""Check if loopback and multicast IP addresses are found."""
has_loopback, has_non_loopback, has_multicast = False, False, False
for _, iface in ifcfg.interfaces().items():
for _, iface in ifcfg_ifaces.items():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't need the keys instead of calling .items() call .values().

Same below.

Signed-off-by: claireyywang <clairewang@openrobotics.org>
@claireyywang
Copy link
Contributor Author

Linux Build Status
Linux-aarch64 Build Status
OSX Build Status
Windows Build Status

@claireyywang claireyywang merged commit 6cd4868 into master Nov 20, 2019
@delete-merged-branch delete-merged-branch bot deleted the claire/update-network-check branch November 20, 2019 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ros2doctor] Issues with output on Windows
4 participants