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

add a few less used canonical mappings #1177

Merged
merged 3 commits into from
Apr 16, 2020

Conversation

itdependsnetworks
Copy link
Contributor

I had this tied to a PR that never got merged, just going back and cleaning up and adding back.

@coveralls
Copy link

coveralls commented Apr 16, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling ce01c90 on itdependsnetworks:can_map into 7ffb3fc on napalm-automation:develop.

@ktbyers
Copy link
Contributor

ktbyers commented Apr 16, 2020

@itdependsnetworks CI-CD is failing. It looks like one test needs updated in the test data such that it uses the canonical reults.

E           AssertionError: Expected result varies on some keys [{"interface": {"result": "Ethernet1/2", "expected": "eth1/2"}}, {"interface": {"result": "Ethernet1/1", "expected": "eth1/1"}}]

Copy link
Member

@mirceaulinic mirceaulinic left a 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()))
Copy link
Member

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

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 do not see this anywhere else in the nxos/nxos_ssh code. I can add it if desired.

Copy link
Contributor

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).

@ktbyers ktbyers merged commit 2eaf132 into napalm-automation:develop Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants