-
Notifications
You must be signed in to change notification settings - Fork 557
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
add a few less used canonical mappings #1177
Conversation
@itdependsnetworks CI-CD is failing. It looks like one test needs updated in the test data such that it uses the canonical reults.
|
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.
LGTM in principle, but would like to clarify with Kirk a matter of detail.
else: | ||
vlans.append(vls.strip()) | ||
vlans.append(napalm.base.helpers.canonical_interface_name(vls.strip())) |
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 like this, just wanted to mention that on IOS at least there's an optional argument canonical_int
defaulting to False
currently - is this something we'd want on NX-OS as well? My preference would probably be to always use the canonical map, but maybe there are some reasons on the existence of canonical_int
arg I'm not aware of. CC @ktbyers
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 do not see this anywhere else in the nxos/nxos_ssh code. I can add it if desired.
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.
Let's open that as a separate issue (so this PR doesn't get bogged down). I would guess for backwards compatibility, but it doesn't look like it was fully/consistently implemented.
I would vote always use canonical interface and just get rid of the argument to toggle that off (especially given that it looks like it is currently inconsistently implemented).
I had this tied to a PR that never got merged, just going back and cleaning up and adding back.