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 mtu to get_interfaces for IOS #529

Closed
wants to merge 5 commits into from
Closed

add mtu to get_interfaces for IOS #529

wants to merge 5 commits into from

Conversation

afourmy
Copy link
Contributor

@afourmy afourmy commented Nov 17, 2017

add mtu to get_interfaces

@afourmy
Copy link
Contributor Author

afourmy commented Nov 17, 2017

fixing the tests now

@afourmy
Copy link
Contributor Author

afourmy commented Nov 17, 2017

OK so it seems that the tests for this PR cannot pass. It was failing because of assert helpers.test_model(models.interface, interface_data), so I updated the keys in base/test/models.py.
Now all the other drivers are failing for the same reason.
The tests are passing for IOS.

@ktbyers
Copy link
Contributor

ktbyers commented Nov 17, 2017

@afourmy If get_interfaces is going to return the MTU, then it has to return the MTU for all of the core platforms (IOS, NX-OS, EOS, IOS-XR, Junos).

That is a requirement of napalm (i.e. the data structure returned is the same across all platforms).

@afourmy
Copy link
Contributor Author

afourmy commented Nov 17, 2017

Yes, it was already done for Junos so I did it for IOS, but I understand that it will need to be updated for all drivers. Can it be done in separate PR that will be merged all at once, or does everything need to be in one PR ?

@ktbyers
Copy link
Contributor

ktbyers commented Nov 17, 2017

@afourmy How about we do this, I will create a new branch get_interfaces_mtu. You submit your PR against this branch.

That way we can incorporate the PRs into this branch platform-by-platform. Once all of the platforms are done, we can submit a PR from this branch to develop.

Does that sound reasonable?

@afourmy
Copy link
Contributor Author

afourmy commented Nov 17, 2017

Yes sounds good, thank you ! @ckishimo I guess you'll have to do the same things for Junos i.e resubmit #516.

@ktbyers
Copy link
Contributor

ktbyers commented Nov 17, 2017

@afourmy @ckishimo I will see if I can do them both (potentially).

@ktbyers ktbyers mentioned this pull request Nov 18, 2017
@ktbyers
Copy link
Contributor

ktbyers commented Nov 18, 2017

Unified into a new branch.

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