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

Fix for issue #1069 #1346

Merged
merged 6 commits into from
Jan 5, 2021
Merged

Fix for issue #1069 #1346

merged 6 commits into from
Jan 5, 2021

Conversation

malanovo
Copy link
Contributor

Fix for issue #1069 . The problem was caused when an empty IP address was passed to one of the helpers due to it not being in the expected location. For certain BGP related routes, peerEntry is moved up a level, so if it isn't found under routeDetail we catch the error thrown by the IP helper and try again in the other location.

@coveralls
Copy link

coveralls commented Dec 29, 2020

Coverage Status

Coverage increased (+0.007%) to 81.078% when pulling 0fb9fcd on malanovo:bugfix-1069 into c3b8222 on napalm-automation:develop.

.get("peerEntry", {})
.get("peerAddr", "")
)
except AddrFormatError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps not a biggie, just curious:

For certain BGP related routes, peerEntry is moved up a level, so if it isn't found under routeDetail we catch the error thrown by the IP helper and try again in the other location.

Does this apply to any specific route types, or any pattern we can distinguish at all? I'd be more inclined to a simpler if 'routeDetail' in bgp_route_details but perhaps there's some other context I'm missing.

Either way, thanks for this patch as well @malanovo and it'd be great to add a test case for this scenario here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I haven't quite figured out yet all the cases when EOS will return peerEntry in the alternate location. The devices I have been testing against will trigger the issue for seemingly similar configuration sections, and I even have two different switches in the same network that look like they should both cause it, yet one does while the other does not. I'm still trying to figure it out, since the values as returned through the CLI seem identical. I'll circle back on it once the thing I'm building that uses this module is finished.

I fully agree with doing the simpler if based method, and originally started doing it that way, but went with the try/except method instead as it was easier to see what was going on, plus it seemed more consistent with existing code (I'm referring to how that method was used elsewhere in the code for catching other instances of AddrFormatError).

I'll work on getting a test case put together. I should be able to capture something from a device in my network to use as a base for it.

@malanovo
Copy link
Contributor Author

malanovo commented Jan 5, 2021

Test case has been added to the pull request.

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.

None yet

3 participants