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

Improve reliability of facts.hardware.NetworkDevices #1098

Merged
merged 4 commits into from May 12, 2024

Conversation

sudoBash418
Copy link
Contributor

I ran into a few issues with NetworkDevices not properly parsing command output.

This PR fixes those issues, and adds a couple of large test cases to match.

Network device names are allowed to contain most characters,
except for `:`, `/`, and whitespace.

Ref: https://elixir.bootlin.com/linux/v6.8.9/source/net/core/dev.c#L1047
The previous regex was matching some IPv6 addresses.
Adding a mandatory `ether ` prefix solves this.

Also fixed the erroneous test cases.
Solves the following issues:
- `ip`: missing IPv4 address when metric is present
- both: missing IPv4 address when broadcast address is not present

Additionally:
- Regexes now use named groups for clarity
- Minor reorganization and some type hints
Based on an Arch Linux machine.

Known issue: the current state parsing behavior is rather imprecise
and is not consistent between `ip a` and `ifconfig -a`.
For example, the `wlp6s0` interface in the `linux_ifconfig_multiple`
test case should have a state of `DOWN`, not `UNKNOWN`.
@sudoBash418
Copy link
Contributor Author

Fixed the mypy issue, not sure why the unit test failed though :/

Copy link
Member

@Fizzadar Fizzadar left a comment

Choose a reason for hiding this comment

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

This is incredible work @sudoBash418, thank you for fixing and cleaning up this one!

@Fizzadar Fizzadar merged commit 105a29b into pyinfra-dev:3.x May 12, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants