-
Notifications
You must be signed in to change notification settings - Fork 756
Fix XR7 parse error where ARP EVPN line exists #2092
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
base: master
Are you sure you want to change the base?
Conversation
Hi @garymccann
Thank you! |
hey @mjbear - can you please break these adjustments into their own PR - i do not like touching files unrelated to the feature, increasing the blast radius and while i don't see anything wrong with changing the order, it may confuse folk who are using the raw data, since it does not follow any alphabetical ordering. I will revert these PR changes. |
@garymccann
|
57a329c
to
fb33403
Compare
^\s+ARP EVPN.*$$ | ||
^\s+ICMP redirects.*$$ | ||
^\s+ICMP unreachables.*$$ | ||
^\s+ICMP mask.*$$ |
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.
Could we please replace literal whitespaces with regex?
(We can leave the other regexes alone on this PR and I can follow up with another PR for other things as mentioned in earlier discussion.)
^\s+ARP EVPN.*$$ | |
^\s+ICMP redirects.*$$ | |
^\s+ICMP unreachables.*$$ | |
^\s+ICMP mask.*$$ | |
^\s+ARP\s+EVPN.*$$ | |
^\s+ICMP\s+redirects.*$$ | |
^\s+ICMP\s+unreachables.*$$ | |
^\s+ICMP\s+mask.*$$ |
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.
These lines are not equivalent. \s+ matches one or more whitespace characters, where the literal whitespace is just one.
I checked performance of literal, \s and \s+ - all 3 take 14 processing steps (regex101.com) so there is no processing gain to be had from any change in implementation.
My personal preference is to keep the literal whitespace, since it reads most similar to the CLI output, however, given there is no performance penalties, i am agnostic.
Please let me know your thoughts, main thing here is to get this merged into a pypy release for my use in production uses.
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'm looking at this through a future-proofing lens -- balancing the flexibility of the template and its regexes to hopefully reduce the maintenance burden in the long term.
I certainly hope that Cisco doesn't add extra accidental whitespace, but we've seen other vendors make somewhat haphazard changes (ex: spacing, capitalization, spelling).
Note
Since I already have it on my mind to tackle putting the structured data alphabetic ordering in a different PR, this literal whitespace could be something that happens there.
My gut feeling is for me to hold off with the whitespace regex, etc until a later PR (as in not for this PR).
I welcome feedback from the other maintainers if they have thoughts on it.
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.
@itdependsnetworks @jmcgill298 @jvanderaa
👋
Thoughts on moving forward with merging this as it is?
Fixes an IOS-XR version 7 issue, where a line is now included for ARP EVPN