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: Account for additional lines in cisco_nxos_bgp_summary #1877

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

asik8888
Copy link

@asik8888 asik8888 commented Oct 14, 2024

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
Copy link
Contributor

mjbear commented Oct 14, 2024

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. 👍

Copy link
Contributor

@mjbear mjbear left a comment

Choose a reason for hiding this comment

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

Regarding the first suggestion, it's more readable to put a space between the # symbol and the comment.

On the second suggestion, it's best to use regexes to account for white space just in case it was to ever change down the road.

(Flexibility by way of regexes means we hopefully shouldn't find it break often as vendors possibly change their output formatting. It's also a middle ground ⚖️ between flexibility and making the regexes overly complex.)

asik8888 and others added 2 commits October 14, 2024 14:48
Co-authored-by: Michael Bear <[email protected]>
Co-authored-by: Michael Bear <[email protected]>
@asik8888
Copy link
Author

asik8888 commented Oct 14, 2024

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
Copy link
Contributor

mjbear commented Oct 14, 2024

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. 😅

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
Copy link
Contributor

mjbear commented Oct 14, 2024

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. 😅

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.

new raw data for the additional checks on cisco_nxos_show_ip_bgp_summary.textfsm
new updated yml file from raw data in same folder
@asik8888 asik8888 changed the title FIX: BGP into lines FIX: Account for additional lines in cisco_nxos_bgp_summary Oct 18, 2024
@asik8888
Copy link
Author

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
Copy link
Contributor

mjbear commented Oct 18, 2024

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) 😉

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).

💡 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants