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 for issue #286 - traceback when banner section contains white-spaces lines #291

Merged
merged 6 commits into from
Dec 6, 2023
Merged

fix for issue #286 - traceback when banner section contains white-spaces lines #291

merged 6 commits into from
Dec 6, 2023

Conversation

dlyssenko
Copy link
Contributor

this PR fixes issue #286

The issue has been introduced with PR#220 - that PR introduced cached parsing of the running config, which is mostly indent-structured text, however, we entirely overlooked a banner section.
Unlike the rest of the config, the banner section(s) is a free text (i.e. unstructured) and therefore may produce:

  • false positive results
  • failures
  • rogue / bogus / phony sections.

The banner is now parsed into a separate section.

@dlyssenko dlyssenko changed the title fix for issue #286 fix for issue #286 - traceback when banner section contains white-spaces lines Dec 4, 2023
mharista
mharista previously approved these changes Dec 5, 2023
Copy link
Contributor

@mharista mharista left a comment

Choose a reason for hiding this comment

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

Looks good to me. The single updated test file handles all the edge cases?

@dlyssenko
Copy link
Contributor Author

dlyssenko commented Dec 5, 2023

Looks good to me. The single updated test file handles all the edge cases?

hmm, good point. Currently, the updated UT file handles only blank lines (empty and with the white spaces). In order to handle rogue configs, I'd need to come up with the negative test case. Will do soon

@dlyssenko
Copy link
Contributor Author

@mharista, I have added a false positive case to UT fixture file to cover the case, please review

@dlyssenko
Copy link
Contributor Author

I've ran also systest (with the latest update) - all have passed

Copy link
Contributor

@chieto-arista chieto-arista left a comment

Choose a reason for hiding this comment

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

LGTM

@dlyssenko dlyssenko merged commit 2246f66 into arista-eosplus:develop Dec 6, 2023
5 checks passed
@dlyssenko dlyssenko deleted the fix_chunkification branch December 6, 2023 12:24
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.

None yet

3 participants