RT-1.31 bgp_3level_nested_policy_test.go#4936
RT-1.31 bgp_3level_nested_policy_test.go#4936cprabha wants to merge 3 commits intoopenconfig:mainfrom
Conversation
Summary of ChangesHello @cprabha, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the BGP 3-level nested policy tests by streamlining policy configurations and enhancing test reliability. The changes ensure that BGP routes are handled correctly by default policies, prevent test interference through robust cleanup mechanisms, and validate specific policy behaviors like invert route filtering. These updates contribute to more accurate and stable testing of BGP routing policies. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable improvements to the BGP nested policy test. The addition of cleanup logic for each subtest significantly enhances test isolation and reliability. The switch from an explicit PERMIT-ALL policy to using default BGP policies simplifies the configuration and aligns better with real-world scenarios. The corrections to policy actions (using ACCEPT_ROUTE instead of NEXT_STATEMENT) and the explicit configuration of default import policies address key behavioral aspects of BGP routing. My review includes suggestions to address violations of the repository's IP addressing style guide and to refactor duplicated cleanup functions for better maintainability.
feature/bgp/policybase/otg_tests/bgp_3level_nested_policy_test/bgp_3level_nested_policy_test.go
Show resolved
Hide resolved
feature/bgp/policybase/otg_tests/bgp_3level_nested_policy_test/bgp_3level_nested_policy_test.go
Show resolved
Hide resolved
feature/bgp/policybase/otg_tests/bgp_3level_nested_policy_test/bgp_3level_nested_policy_test.go
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 20256658540Details
💛 - Coveralls |
|
@ram-mac Please could you review and help to merge this git pull request. |
Hi,
Below are the modifications done in RT-1.31.
1.) PERMIT ALL route policy is not required, we can enable default accept policy, this will only allow BGP routes.
Explicit PERMIT ALL route policy will allow non bgp routes as well.
2.) Added clean up policy after each test , in case if any of subtest fails, it does not retain policy and that will cause failure in next sub testcase.
3.) Enabled one prefix in invert route filter policy so make sure invert works fine for required prefix.
4.) In each of nested policy we need ACCEPT (this is similar to RT-7.11, please check RT-7.11)
5.) By default, Ebgp does not accept routes , default value is REJECT. Hence explicit ACCEPT is needed to import routes.
Thanks,
Prabha