networktocode / ntc-templates

TextFSM templates for parsing show commands of network devices
https://ntc-templates.readthedocs.io/
Other
1.12k stars 730 forks source link

FIX: Account for additional lines in cisco_nxos_bgp_summary #1877

Closed asik8888 closed 1 month ago

asik8888 commented 1 month ago

Few lines in cisco_nxos_show_ip_bgp_summary output error out while parsing. The following changes can help with reading those additional lines seen in "show ip bgp summary" commands.

Files modified: cisco_nxos_show_ip_bgp_summary.textfsm cisco_nxos_show_ip_bgp_summary.raw cisco_nxos_show_ip_bgp_summary.yml

mjbear commented 1 month ago

Hello @asik8888 Thank you for opening a PR. The community here was welcoming to me so I intend to pay it forward here.

There are a few items missing from your PR that mean it needs some work before the maintainers will consider merging it. Please allow me to explain.

Please provide a PR description. It is important to provide a brief summary of what your changes accomplish.

There are several bits of documentation in dev docs and user docs Specifically:

I hope this helps guide you. If anything needs clarified after you've looked at the documentation, please ask. :+1:

asik8888 commented 1 month ago

Hello @mjbear,

Thank you for the helpful suggestions, I appreciate your input in helping me. I have resolved the conflicts from the above two suggestions and added spacing using regex to two existing lines. Please let me know if I can do anything further for this PR

mjbear commented 1 month ago

Hello @mjbear,

Thank you for the helpful suggestions, I appreciate your input in helping me. I have resolved the conflicts from the above two suggestions and added spacing using regex to two existing lines. Please let me know if I can do anything further for this PR

Great.

Get a copy of the raw test data that the template had issues with. It doesn't have to be a mile long, but it should include the unique patterns -- though that said for bgp summary the file won't be miles long. :sweat_smile:

Note: If you need to, change up IP addresses to hide any publicly routable space (or anything that's "sensitive"). (ex: 123.123.123.123 could become 10.0.0.123)

Put that raw data in a new file in tests/cisco_nxos/show_ip_bgp_summary. You can then use the cli.py helper script to generate the yaml file.

Once that's done, run your changes through the test suite via invoke before pushing your updates to GitHub.

mjbear commented 1 month ago

Few lines in "show ip bgp summary" output error out while parsing due to the following lines not being handled correctly in the template. These changes can help with reading those additional lines seen in "show ip bgp summary" commands.

@asik8888 You might update the title and description a bit to call out which template.

It's tough to really tell without the test data so I'll probably botch my "title" suggestion below until I see the test data. :sweat_smile:

Title: Account for additional lines in nxos bgp summary

Body: Few lines in cisco_nxos "show ip bgp summary" output error out while parsing due to the following lines not being handled correctly in the template. These changes can help with reading those additional lines seen in "show ip bgp summary" commands.

asik8888 commented 1 month ago

Hello @mjbear,

Added the raw data and yml file to the tests folder. Invoke test, passed. test

Updated the title and body. Let me know if you would like it in a different format or any other changes.

Thank you for the help!

mjbear commented 1 month ago

Hello @mjbear,

Added the raw data and yml file to the tests folder. Invoke test, passed. test

Updated the title and body. Let me know if you would like it in a different format or any other changes.

Thank you for the help!

Hello @asik8888,

Apologies for not providing more guidance about the test data. Adding new test files is oftentimes better than modifying existing ones. (more on that below) :wink:

I would would strongly recommend retaining the existing tests/cisco_nxos/show_ip_bgp_summary/cisco_nxos_show_ip_bgp_summary.raw in its original form.

Then from there add another file with your new test data (example: tests/cisco_nxos/show_ip_bgp_summary/cisco_nxos_show_ip_bgp_summary2.raw).

:bulb: This way the test data includes not only the old example, but also your new example. This helps provide a broader test base so the project testing has a chance at catching any unexpected errors/regressions.

Hope that helps!

mjbear commented 1 month ago

@asik8888 I hope you're doing well.

I dropped a PR over on your fork so you should be able to easily merge in those tweaks and update this PR. Please review https://github.com/asik8888/ntc-templates/pull/2 when you have a chance. Thank you!

asik8888 commented 1 month ago

Thank you for input and creating a pull request. sorry for the delay in adding the required files in order. Got busy on something else. I apologize. Have merged the pull request.

asik8888 commented 3 weeks ago

Hi @jmcgill298

was wondering when this will be released?

mjbear commented 3 weeks ago

Hi @jmcgill298

was wondering when this will be released?

Hello @asik8888 I'll provide some observations here, but I can't speak for the NTC team as to when the next ntc-templates release will be.

The changes you brought forward were merged into the master branch so it is available, but just not yet part of a versioned release. You could git clone the repo and point your scripts at that clone if you chose to.

Now as a longer term item a release is certainly more desirable. Seeing as how the last release (7.3.0) was two weeks ago it is possible (though no guarantee) that a release may be coming in a week or two.

asik8888 commented 3 weeks ago

Thank you for the update @Michael bear. Appreciate it

On Mon, Nov 4, 2024 at 6:27 AM Michael Bear @.***> wrote:

Hi @jmcgill298 https://github.com/jmcgill298

was wondering when this will be released?

Hello @asik8888 https://github.com/asik8888 I'll provide some observations here, but I can't speak for the NTC team as to when the next ntc-templates release will be.

The changes you brought forward were merged into the master branch so it is available, but just not yet part of a versioned release. You could git clone the repo and point your scripts at that clone if you chose to.

Now as a longer term item a release is certainly more desirable. Seeing as how the last release (7.3.0) was two weeks ago https://github.com/networktocode/ntc-templates/releases/tag/v7.3.0 it is possible (though no guarantee) that a release may be coming in a week or two.

— Reply to this email directly, view it on GitHub https://github.com/networktocode/ntc-templates/pull/1877#issuecomment-2454586302, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJBKCTV7XHQTG524VDERZA3Z65KYRAVCNFSM6AAAAABP5PJ7FSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJUGU4DMMZQGI . You are receiving this because you were mentioned.Message ID: @.***>

jmcgill298 commented 3 weeks ago

its out now

asik8888 commented 3 weeks ago

Thank you Jacob!

On Mon, Nov 4, 2024 at 9:43 AM Jacob McGill @.***> wrote:

its out now

— Reply to this email directly, view it on GitHub https://github.com/networktocode/ntc-templates/pull/1877#issuecomment-2455052504, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJBKCTQDTTRMBASTFF3SYDLZ66BZHAVCNFSM6AAAAABP5PJ7FSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJVGA2TENJQGQ . You are receiving this because you were mentioned.Message ID: @.***>