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

iss-1416 #1573

Conversation

emfusion
Copy link
Contributor

  • fix get_bgp_neighbors_detail() when using multi-agent mode
  • fix issue where multihop var was wrongly interpreted as it was comparing 0 with ‘0’
  • Add IPv6 prefixes to the advertised and received prefix count
  • Add more test for get_bgp_neighbors_detail

Anthony Iheoma added 3 commits February 17, 2022 12:18
Move the multi-agent check from the open() (globally set) to the
get_bgp_neighbors_detail() function (locally set).

Update the bgp_detail_multi_agent textfsm template to:
- Fix issue where the connection_state and local_address were sometimes missing
- Use a newline as a separator for each record

Fix issue where multihop var was wrongly interpreted as it was comparing 0 with ‘0’

Add IPv6 prefixes to the advertised and received prefix count. Update the
testcase and bgp_detail.tpl textfsm template to support this

Add more test for get_bgp_neighbors_detail
napalm/eos/eos.py Outdated Show resolved Hide resolved
napalm/eos/eos.py Outdated Show resolved Hide resolved
napalm/eos/eos.py Outdated Show resolved Hide resolved
napalm/eos/eos.py Outdated Show resolved Hide resolved
napalm/eos/eos.py Show resolved Hide resolved
@emfusion emfusion force-pushed the wip-1416-add-multiagent-support branch from db8f00f to 32389c0 Compare February 21, 2022 10:58
@mirceaulinic mirceaulinic added this to the 4.0.0 milestone Mar 25, 2022
Copy link
Member

@mirceaulinic mirceaulinic 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 overall, only had one question and a minor nit.

napalm/eos/eos.py Show resolved Hide resolved
napalm/eos/eos.py Outdated Show resolved Hide resolved
@bewing
Copy link
Member

bewing commented Apr 5, 2022

Little late the the party here, apologies.

Are the two output formats so different we can't use a single template? Recognizing the start of the next record should allow one to close out the previous record -- this approach is used in the VRF template: https://github.com/napalm-automation/napalm/blob/develop/napalm/eos/utils/textfsm_templates/vrf.tpl#L7

Removing the -> Next.Record line at the end and adding

BGP neighbor is \S+, remote AS \d+, .* -> Continue.Record

as the first regexp might allow us to use a single template, if the rest of the output is similar enough? We would need to specify at least one required variable, so that empty records aren't recorded.

@mirceaulinic
Copy link
Member

@emfusion have you had a chance to look at Brandon's suggestion above? This still LGTM otherwise...

@mirceaulinic mirceaulinic merged commit dd682c7 into napalm-automation:develop Jun 3, 2022
@mirceaulinic
Copy link
Member

Merged as-is, but feel free to follow up with another PR and we shall review! 😄

@emfusion
Copy link
Contributor Author

emfusion commented Jun 4, 2022

@emfusion have you had a chance to look at Brandon's suggestion above? This still LGTM otherwise...

Hi, I unfortunately havent had the time to look into it (forgot how time consuming newborns are 😆). It's on my To Do list and will definitely jump into it soon

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

Successfully merging this pull request may close these issues.

None yet

4 participants