modified hp_comware_display_interface.textfsm to account for lowercase "maximum frame length" #1982
Conversation
…ally, added appopriate test files to prove changes are working for both backwards compatability as well as new test scenario This commit is a squash commit. Squashed commit of the following: commit 388041cb0941bc27caadbaa931854d3f7b354f60 Author: Cain,Marcus <[email protected]> Date: Wed Jan 8 12:30:36 2025 -0500 Including additional test to show that backward compatibility has been maintained Signed-off-by: Cain,Marcus <[email protected]> commit fc6cc6540ff77e9294927e8f3cdf9341cd37a560 Author: Cain,Marcus <[email protected]> Date: Mon Jan 6 15:08:01 2025 -0500 modified textfsm template to account for lowercase mtu line. Additionally, added appopriate test files to prove changes are working Signed-off-by: Cain,Marcus <[email protected]> Signed-off-by: Cain,Marcus <[email protected]>
marcus-cain
requested review from
itdependsnetworks,
jmcgill298 and
jvanderaa
as code owners
| @@ -28,7 +28,7 @@ Start | |||
| ^\s*The\sMaximum\s+Transmit\s+Unit\sis\s+${MTU} | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
I agree, your suggested changes would make this more future proof, since it would account for a sinlge white-space (which is the currently logic) and/or multiple instances of a white-space. HP can be very inconsistent in the way they output the data, so i think its a good idea to incorporate \s+
Now, my internal process works a little bit different:
[internal fork ] -------------------- [ public fork Fidelity-Contributions] --------------- upstream repo (ntc-templates)
the github repo that is actually making the PR is "Fidelity-Contributions" as you can see above... This is the public facing repo that i do not have write access to.... so i would have to make changes in my internal fork first, get it reviewed and approved internally (shouldn't be too bad, since its not any major changes), and then once reapproved, it will be synced to the public facing repo Fidelity-Contributions... Then i can resubmit the PR.
So - i could do this.. or i can fix this on the next PR.. I am getting ready to open another issue on this same template that i recently uncovered for 100G interfaces. Thus, i could make the next PR to directly solve the new issue AND future-proof the white space..
Fine w/ either way. Just let me know.
There was a problem hiding this comment.
Now, my internal process works a little bit different:
[internal fork ] -------------------- [ public fork Fidelity-Contributions] --------------- upstream repo (ntc-templates)
the github repo that is actually making the PR is "Fidelity-Contributions" as you can see above... This is the public facing repo that i do not have write access to.... so i would have to make changes in my internal fork first, get it reviewed and approved internally (shouldn't be too bad, since its not any major changes), and then once reapproved, it will be synced to the public facing repo Fidelity-Contributions... Then i can resubmit the PR.
I appreciate the insight and explanation into your corporate process.
Ah, the internal fork bit limits the collaborative process.
Ok - what I'll do here is remove my suggestions from this PR and create a separate PR. instead add to PR #1963 once this PR is merged and I'm able to rebase.
If there's ever something I submit forth that you want to make tweaks to I'll consider them!
| ^\s*The\s[Mm]aximum\s+[Ff]rame\s+[Ll]ength\sis\s+${L2MTU} | ||
| ^\s*Internet\s+[Aa]ddress:\s+${IP_ADDRESS}\s+\([Pp]rimary\) | ||
| ^\s*Internet\s+[Aa]ddress\sis\s+${IP_ADDRESS}\s+[Pp]rimary |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Heads up that #1963 is yet to be merged, but also makes changes to this template. And there's a direct conflict with the test data file names with 1963. |
That would be helpful.. based on the comments in #1963, seems like it may take a little bit for him to incorporate the change. So this PR might be merged first??.. Thus in that case, the contributor in 1963 can follow the standard process of pulling in the latest changes and rebasing his branch off of the latest, and resolve any conflicts if needed. In the future, i can make the names of the test file be more indicative of what is being tested/fixed, (which i saw you did earlier), instead of following the generic 1,2,3....10 naming convention. |
I'm not indicating merge order, but I will drop a note to indicate we likely want to merge your PR here first since you have corporate re-approvals to go through whereas the other PR does not (or doesn't appear to).
Whether that contributor solves the possible merge conflicts or whether I choose to step up to resolve them isn't determined yet. 😀
Makes sense. Less chance for a file name conflict, hehe. |
|
@itdependsnetworks @jmcgill298 @jvanderaa What I'm thinking is I'll rebase/resolve any conflicts and add extra polish to 1963. |
modified textfsm template to account for lowercase variation of the mtu line. Additionally, added appropriate test files to prove changes are working
fixes #1960