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 regex and both for import and export route-targets NXOS #843

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BingPB
Copy link

@BingPB BingPB commented Apr 26, 2024

Description

I have changed the regex pattern for matching route-target to include ':'.
A ':' is required when configuring a import/export route-target.

I have also fixed, so a route-target can be 'both' imported and exported at the same time.

Motivation and Context

This fixes the "show running-config vrf {vrf} | sec '^vrf'" command on NXOS.
Before route-targets would not be parsed, and not appear in parsed output.

Impact (If any)

No negative impact, as this did not work previously.

Screenshots:

Checklist:

  • I have updated the changelog.
  • I have updated the documentation (If applicable).
  • I have added tests to cover my changes (If applicable).
  • All new and existing tests passed.
  • All new code passed compilation.

@BingPB BingPB requested a review from a team as a code owner April 26, 2024 10:16
@BingPB BingPB requested review from Taarini and lsheikal April 26, 2024 10:16
@BingPB
Copy link
Author

BingPB commented May 14, 2024

Hello, any update on this? Would be nice to have the library fixed.
@Taarini @lsheikal

Copy link
Contributor

@Taarini Taarini left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Sorry for not reviewing it sooner

Could you also add UT

@@ -292,7 +292,7 @@ def cli(self, vrf=None):
# route-target both auto
# route-target both auto mvpn
# route-target both auto evpn
p5 = re.compile(r'^\s*route-target +(?P<rt_type>\w+) +(?P<rt>\w+)( +(?P<rt_evpn_mvpn>\w+))?$')
p5 = re.compile(r'^\s*route-target +(?P<rt_type>\w+) +(?P<rt>\w+:\w+)( +(?P<rt_evpn_mvpn>\w+))?$')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comment for the regex change

Copy link
Author

@BingPB BingPB Aug 8, 2024

Choose a reason for hiding this comment

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

Sorry, but I'm not sure what comment to add, since it should have been like this from the start.
Also UT = Usertest? Just some examples, from before and after the change?

I'm till very interested in getting this fixed.

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