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

Updating techsupport commands to include optimized bgp neighbor commands #15466

Merged
merged 9 commits into from
Jan 2, 2025

Conversation

dgsudharsan
Copy link
Contributor

@dgsudharsan dgsudharsan commented Nov 8, 2024

Description of PR

Summary:
Fixes # (issue)

DO NOT MERGE until sonic-net/sonic-utilities#3605 is merged and submodule is updated.
Update techsupport test to align with this PR sonic-net/sonic-utilities#3605

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405

Approach

What is the motivation for this PR?

To support covering show techsupport after the enhancement introduced in sonic-net/sonic-utilities#3605

How did you do it?

Used regex to modify the validations to align with the new optimized vtysh command in the show techsupport

How did you verify/test it?

Running the test with sonic-utilities changes

Any platform specific information?

This is platfrm agnostic chhanges.

Supported testbed topology if it's a new test case?

Not a new test case

Documentation

@mssonicbld
Copy link
Collaborator

The pre-commit check detected issues in the files touched by this pull request.
The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed
check python ast.........................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

tests/show_techsupport/tech_support_cmds.py:119:121: E501 line too long (121 > 120 characters)

flake8...............................................(no files to check)Skipped
check conditional mark sort..........................(no files to check)Skipped

To run the pre-commit checks locally, you can follow below steps:

  1. Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
    the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
    docker container.
  2. Ensure that the pre-commit package is installed:
sudo pip install pre-commit
  1. Go to repository root folder
  2. Install the pre-commit hooks:
pre-commit install
  1. Use pre-commit to check staged file:
pre-commit
  1. Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

@mssonicbld
Copy link
Collaborator

The pre-commit check detected issues in the files touched by this pull request.
The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...........................................(no files to check)Skipped
check for added large files..............................................Passed
check python ast.........................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

tests/show_techsupport/tech_support_cmds.py:118:77: E502 the backslash is redundant between brackets
tests/show_techsupport/tech_support_cmds.py:119:17: E127 continuation line over-indented for visual indent
tests/show_techsupport/tech_support_cmds.py:120:79: E502 the backslash is redundant between brackets
tests/show_techsupport/tech_support_cmds.py:121:17: E127 continuation line over-indented for visual indent

flake8...............................................(no files to check)Skipped
check conditional mark sort..........................(no files to check)Skipped

To run the pre-commit checks locally, you can follow below steps:

  1. Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
    the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
    docker container.
  2. Ensure that the pre-commit package is installed:
sudo pip install pre-commit
  1. Go to repository root folder
  2. Install the pre-commit hooks:
pre-commit install
  1. Use pre-commit to check staged file:
pre-commit
  1. Alternatively, you can check committed files using:
pre-commit run --from-ref <commit_id> --to-ref <commit_id>

re.compile(r'vtysh{}\s+-c "show ip bgp neighbors .* routes"'),
re.compile(r'vtysh{}\s+-c "show bgp ipv6 neighbors .* advertised-routes"'),
re.compile(r'vtysh{}\s+-c "show bgp ipv6 neighbors .* routes"'),
re.compile(r"vtysh(\s+-Ec 'show bgp ipv4 neighbors .* advertised-routes' "
Copy link
Contributor

Choose a reason for hiding this comment

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

does wildcard support here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify your question? It does work with the changes in the sonic-utilities PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

no issue, I just remember this was not supported, sorry maybe I missed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please approve if all good?

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@FengPan-Frank FengPan-Frank left a comment

Choose a reason for hiding this comment

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

LGTM, pls fix pre-checkin test issue.

@dgsudharsan
Copy link
Contributor Author

/azpw run Azure.sonic-mgmt

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-mgmt

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgsudharsan
Copy link
Contributor Author

/azpw run Azure.sonic-mgmt

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-mgmt

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

wangxin pushed a commit that referenced this pull request Dec 23, 2024
This PR should be followed by #15466.

There is a circular dependency where the sonic-utilities submodule after this change sonic-net/sonic-utilities#3605 will not get update unless show techsupport test pass. However the test cannot pass until we have sonic-utilities submodule updated.

To overcome this create a temporary PR to remove the techsupport commands that create this dependency. After the submodule update is done #15466 can be merged
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgsudharsan
Copy link
Contributor Author

Test failed because the test in run with "build_version": "master.728471-af18b144e",
which doesn't contain the required sonic-utilities changes. Once the master build is updated in pipelines the test should pass

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dgsudharsan
Copy link
Contributor Author

@yxieca @maipbui @FengPan-Frank Can you please help merge?

@yxieca
Copy link
Collaborator

yxieca commented Jan 2, 2025

@dgsudharsan can you please update the PR template:

What is the motivation for this PR?
How did you do it?
How did you verify/test it?
Any platform specific information?
Supported testbed topology if it's a new test case?

@dgsudharsan
Copy link
Contributor Author

@yxieca Done.

@yxieca yxieca merged commit 6f45beb into sonic-net:master Jan 2, 2025
17 checks passed
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.

6 participants