-
Notifications
You must be signed in to change notification settings - Fork 555
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
Adds mac, ip checks for getters #1560
Conversation
…ail, get_interfaces_ip, get_ntp_peers, traceroute, ping
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.
It feels great align everything! Two changes requested, then should be able to get this in.
napalm/ios/ios.py
Outdated
# format chassis_id for consistency, if it is a mac address | ||
for entry in lldp_entries: | ||
if re.search( | ||
r"^(\d|\w){4}.(\d|\w){4}.(\d|\w){4}$", entry["remote_chassis_id"] |
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.
Instead of doing this regex check, I'd just apply the convert
function instead, e.g.,
r"^(\d|\w){4}.(\d|\w){4}.(\d|\w){4}$", entry["remote_chassis_id"] | |
entry["remote_chassis_id"] = napalm.base.helpers.conver(napalm.base.helpers.mac, entry["remote_chassis_id"], entry["remote_chassis_id"]) |
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.
Thanks for the quick review and all the suggestions. Just checking: I agree with your suggestion, but if the chassis id cannot be converted, an empty chassis id will be returned. This is an example value from a chassis id from one of the tests:
In [75]: id = "hmbbkku-la"
In [76]: id = napalm.base.helpers.convert(napalm.base.helpers.mac, id)
In [77]: id
Out[77]: ''
Is that OK?
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 have addressed the comment with the latest commit. Let me know what you think @mirceaulinic
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.
Yes, there should be a third argument, with the default, notice in the suggestion above:
entry["remote_chassis_id"] = napalm.base.helpers.conver(napalm.base.helpers.mac, entry["remote_chassis_id"], entry["remote_chassis_id"])
When unable to convert to the normalised MAC format, it'd default to the extracted value.
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.
Thank you for the help! Added.
Co-authored-by: Mircea Ulinic <mirceaulinic@users.noreply.github.com>
Thank you @mundruid! |
Adds mac / ip checks for:
Covers #451