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(eos_cli_config_gen): Fix typo in router-bgp.j2 #2753

Merged

Conversation

adietrich-ussignal
Copy link
Contributor

@adietrich-ussignal adietrich-ussignal commented Apr 22, 2023

Change Summary

Fixes a typo in the router-bgp.j2 template of eos_cli_config_gen to resolve issues when bfd is set to False under the neighbor configuration.

Related Issue(s)

Fixes #2752

Component(s) name

arista.avd.eos_cli_config_gen

Proposed changes

Revise {{ neighbo.ip_address }} to {{ neighbor.ip_address }}

How to test

Added both BFD True and False use cases to test tenant configurations in the 'evpn_underlay_ebgp_overlay_ebgp' molecule within the bgp_peers configuration section.

Checklist

User Checklist

  • N/A

Repository Checklist

  • My code has been rebased from devel before I start
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation and documentation have been updated accordingly.
  • I have updated molecule CI testing accordingly. (check the box if not applicable)

@adietrich-ussignal adietrich-ussignal requested a review from a team as a code owner April 22, 2023 21:21
@github-actions github-actions bot added role: eos_cli_config_gen issue related to eos_cli_config_gen role state: CI Updated CI scenario have been updated in the PR labels Apr 22, 2023
Copy link
Contributor

@gmuloc gmuloc left a comment

Choose a reason for hiding this comment

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

Thanks a lot @adietrich-ussignal for catching this and submitting a PR to fix it!

Could you please add the test for bfd: false in the eos_designs_unit_test scenario - if you have a look in TenantA, bfd: true is being tested already: https://github.com/aristanetworks/ansible-avd/blob/devel/ansible_collections/arista/avd/molecule/eos_designs_unit_tests/inventory/group_vars/DC1_TENANTS_NETWORKS/Tenant_A.yml#L179

Thanks in advance!

@gmuloc gmuloc changed the title (eos_cli_config_gen) Fix typo in router-bgp.j2 Fix(eos_cli_config_gen): Fix typo in router-bgp.j2 Apr 24, 2023
@gmuloc gmuloc requested a review from a team April 24, 2023 07:28
@gmuloc gmuloc added the cherry-pick-for-3.8.x PR to be cherry-picked to releases/v3.8.x brach later label Apr 24, 2023
@gmuloc
Copy link
Contributor

gmuloc commented Apr 24, 2023

@carlbuchmann - we will need to cherry-pick this for 3.8.x

@adietrich-ussignal
Copy link
Contributor Author

@gmuloc I've added the additional test as requested.

@gmuloc
Copy link
Contributor

gmuloc commented Apr 24, 2023

@gmuloc I've added the additional test as requested.

Thanks @adietrich-ussignal - my apologies for not being more clear in my comment - my intention was to move the test to eos_designs_unit_tests and remove it from the other scenario. Let me know if you want to remove it or I can take care of it.

thanks!

@adietrich-ussignal
Copy link
Contributor Author

@gmuloc I believe I cleaned the right tests up this time. Thanks for the clarification!

Copy link
Contributor

@gmuloc gmuloc left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for the contribution

@gmuloc gmuloc requested a review from a team April 24, 2023 13:02
@carlbuchmann carlbuchmann added the type: bug Something isn't working label Apr 25, 2023
@carlbuchmann
Copy link
Member

@adietrich-ussignal - while reviewing with @gmuloc we noticed another behavior at the cli that required fixing:

  • no neighbor {{ neighbor.ip_address }} bfd should only be generated when that device is also defined in a peer_group. This was verified at the CLI with EOS version 4.29.3M.
  • This needed fixing in both the vrf and within the default context.

We also moved all testing to eos_cli_config_gen unit tests.

Copy link
Member

@carlbuchmann carlbuchmann left a comment

Choose a reason for hiding this comment

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

LGTM

@carlbuchmann carlbuchmann merged commit 33ca2dc into aristanetworks:devel Apr 25, 2023
34 checks passed
Shivani-chourasiya pushed a commit to Vibhu-gslab/avd that referenced this pull request Apr 27, 2023
@carlbuchmann carlbuchmann removed the cherry-pick-for-3.8.x PR to be cherry-picked to releases/v3.8.x brach later label May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn: Fix(eos_cli_config_gen) role: eos_cli_config_gen issue related to eos_cli_config_gen role state: CI Updated CI scenario have been updated in the PR type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to disable bfd for BGP neighbor
3 participants