-
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
Rework eos.get_bgp_neighbor
#2116
Conversation
Since we have dropped support for EOS < 4.23, we can use the JSON output from `show ip bgp neighbors` and related commands instead of processing text output. This eliminates many edge cases in testing, so I have re-done unit testing as well.
Test cases removed:
|
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.
One other question, what is the difference between when "global" VRF vs "default" is used on Arista?
test/eos/mocked_data/test_get_bgp_neighbors/issue1759/expected_result.json
Show resolved
Hide resolved
test/eos/mocked_data/test_get_bgp_neighbors/normal/expected_result.json
Outdated
Show resolved
Hide resolved
VRF-aware Arista output uses the the key |
Everything looks good to me (after fixing the uptime to be -1 for BGP peers that are down/admin down). One other thing I saw that was odd in the test data was the remote_id of
Is that just a config error in the BGP setup? |
If a peer is down, you don't know his router ID. Same logic from before this change: https://github.com/napalm-automation/napalm/pull/2116/files#diff-c33f60453329944c74a7bdfb2416f90a5fe0debffbe70a7aefb83a07dc6c81d0L771 Same behavior in other drivers:
|
@bewing Ooops, missed that the BGP peer was down... |
LGTM. |
Use all JSON output for
get_bgp_neighbor
to remove any text-parsing and prevent CLI output updates from breaking the code. Supercedes #2112Since I'm touching the code and the tests, I'd really love some more eyes on this before a merge.