-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(networking): Allow configuring guest network when there is more than one interface attached #13686
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
fix(networking): Allow configuring guest network when there is more than one interface attached #13686
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changeset looks great! All the comments left are simply around styling (and adding a doc header). The styling comments are based on this document which includes some explanations for the various style preferences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Enables guest network configuration when multiple network interfaces are present by parsing multi-line nmcli
output.
- Refactored inline parsing into a dedicated
get_current_devices
method - Updated tests to cover multi-line and empty-line outputs, and improved regex escaping
- Integrated new helper into
configure_network_manager
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
lib/vagrant/util/guest_networks.rb | Extracted NMCLI parsing into get_current_devices and updated configure_network_manager to use it |
test/unit/vagrant/util/guest_networks_spec.rb | Added specs for get_current_devices and improved IP regex matching in existing tests |
Comments suppressed due to low confidence (1)
test/unit/vagrant/util/guest_networks_spec.rb:149
- Add a test case for single-line
nmcli
output (e.g., one device) to ensure the method handles that scenario correctly.
describe ".get_current_devices" do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 👍
This change enables guest network configuration even when multiple network interfaces are present. Previously, the logic for parsing
nmcli -t c show
output assumed only one interface, causing failures when multiple lines were returned. The updated code now correctly handles multi-linenmcli
output by first splitting it into individual lines and then processing only the non-empty ones.Closes: #13685