-
Notifications
You must be signed in to change notification settings - Fork 555
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
[IOS][get_vlans()] Parse VLAN IDs when private VLANs are implemented #1994
base: develop
Are you sure you want to change the base?
Conversation
@mirceaulinic > @vvas1lev it seems like there's a conflict after merging #2010 (sorry!) - could you take took please? Took a quick look but github actions (black) is failing despite it all being OK locally. I'll have another look some time next week. (venv) X@X napalm % black --check . |
@vvas1lev Which version of black do you have installed? It needs to be black verson:
|
@ktbyers that was it... thanks! |
@vvas1lev - FYI the tests are still failing... |
napalm/ios/ios.py
Outdated
continue | ||
else: | ||
vlan_id = vlan_m.group(1) | ||
vlan_name = _vlan_name if _vlan_name else vlan_name |
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.
Probably need to add a strip
call on this line somewhere given the failing test output
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.
I gave this a try, and even with a strip()
the tests would need the following in order to pass with this code:
diff --git a/test/ios/mocked_data/test_get_vlans/no_show_vlan_all_ports/expected_result.json b/test/ios/mocked_data/test_get_vlans/no_show_vlan_all_ports/expected_result.json
index da9cd8e0..ec258ac5 100644
--- a/test/ios/mocked_data/test_get_vlans/no_show_vlan_all_ports/expected_result.json
+++ b/test/ios/mocked_data/test_get_vlans/no_show_vlan_all_ports/expected_result.json
@@ -227,15 +227,13 @@
"2432": {
"name": "Vlan2432",
"interfaces": [
- "GigabitEthernet0/1",
- "GigabitEthernet0/2"
+ "Port-channel1"
]
},
"2433": {
"name": "Vlan2433",
"interfaces": [
- "GigabitEthernet0/3",
- "GigabitEthernet0/4"
+ "Port-channel1"
]
}
}
So I believe there's more work required in this PR, re-scheduling for the next release.
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.
@mirceaulinic @bewing I figured it'd need more work, which is why I changed it back to draft. I'm just back from PTO, will take a look at this coming week.
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.
Cool - thanks for the heads up @vvas1lev!
This change results in properly parsing the primary & secondary VLAN IDs.