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

Add verify.py for iosxe eigrp folder containing 02 APIs for EIGRPv4 and EIGRPv6 #156

Merged
merged 21 commits into from
Mar 13, 2024

Conversation

cherifimehdi
Copy link
Contributor

Hello,

I noticed there is no verify.py file and verify apis for ios/iosxe eigrp protocol

I added verify.py file at pkgs/sdk-pkg/src/genie/libs/sdk/apis/iosxe/eigrp containing 02 APIs for EIGRPv4 and EIGRPv6:

    * verify_eigrp_interfaces
    * verify_eigrp_neighbors

I added all the tests at pkgs/sdk-pkg/src/genie/libs/sdk/apis/tests/iosxe/eigrp/verify containing the test results for the 02 APIs in verify.py

I added the changelog_add_eigrp_verify.py_apis_202405031904 file at pkgs/sdk-pkg/changelog folder

Thank you for reviewing my contribution

Test for verify_eigrp_interfaces api
Add test for verify_eigrp_neighbors api
Add changelog_add_eigrp_verify.py_apis_202405031904.rst for eigrp verify.py
Add verify.py file for eigrp containing 02 APIs
@cherifimehdi
Copy link
Contributor Author

Hello @ThomasJRyan @Lelor can you please tell me why there is a problem in test step? there is Some checks were not successful but for me it is OK since I added previously get.py for EIGRP without any problem. Thank you

@ThomasJRyan
Copy link
Collaborator

image

Looks to be caused by your branch being out of date. We're currently on version 24.2. Pull the latest master branch into yours and all should be good in the world

@cherifimehdi
Copy link
Contributor Author

Thank you @ThomasJRyan finally all checks passed

Copy link
Collaborator

@ThomasJRyan ThomasJRyan left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. Unfortunately I think this PR will need some work before we can merge it in

pkgs/sdk-pkg/src/genie/libs/sdk/apis/iosxe/eigrp/verify.py Outdated Show resolved Hide resolved
pkgs/sdk-pkg/src/genie/libs/sdk/apis/iosxe/eigrp/verify.py Outdated Show resolved Hide resolved
pkgs/sdk-pkg/src/genie/libs/sdk/apis/iosxe/eigrp/verify.py Outdated Show resolved Hide resolved
pkgs/sdk-pkg/src/genie/libs/sdk/apis/iosxe/eigrp/verify.py Outdated Show resolved Hide resolved
pkgs/sdk-pkg/src/genie/libs/sdk/apis/iosxe/eigrp/verify.py Outdated Show resolved Hide resolved
pkgs/sdk-pkg/src/genie/libs/sdk/apis/iosxe/eigrp/verify.py Outdated Show resolved Hide resolved
pkgs/sdk-pkg/src/genie/libs/sdk/apis/iosxe/eigrp/verify.py Outdated Show resolved Hide resolved
pkgs/sdk-pkg/src/genie/libs/sdk/apis/iosxe/eigrp/verify.py Outdated Show resolved Hide resolved
@cherifimehdi
Copy link
Contributor Author

Hi @ThomasJRyan, thanks for reviewing my PR. I went through all the changes and tested it, it works perfectly.
Thanks for reviewing my post.

Some issues resolved concerning
@cherifimehdi
Copy link
Contributor Author

cherifimehdi commented Mar 9, 2024

Hello @ThomasJRyan @Lelor @SohanTirpude are the changes I made ok. Because I addressed all them. Thank you

@cherifimehdi
Copy link
Contributor Author

Hi @ThomasJRyan are the changes I made ok based on your review? Thanks

Copy link
Collaborator

@ThomasJRyan ThomasJRyan left a comment

Choose a reason for hiding this comment

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

I would really appreciate you running verify.py through a formatter like black or similar

Verify.py updated with fixed changes
@cherifimehdi
Copy link
Contributor Author

Hello @ThomasJRyan Thank you for your valuable comments. Now I used Black instead of Pylint and normally it is reformatted. I have made all the necessary changes based on your suggestions and feedback. Thanks for reviewing my PR

Copy link
Collaborator

@ThomasJRyan ThomasJRyan left a comment

Choose a reason for hiding this comment

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

Small things, but nothing worth blocking the PR further over. Thank you for the contribution!

Update verify.py based on suggested changes
@cherifimehdi
Copy link
Contributor Author

Hi @ThomasJRyan Thank you very much for your valuable remarks and taking time to review my PR and accepting my contribution.
By the way I addressed the suggested changes on timeout.sleep().
Thank you sir

@ThomasJRyan
Copy link
Collaborator

No problem! I'll get @Lelor on this review as soon as possible so we can get you all merged in

@ThomasJRyan ThomasJRyan merged commit f332a71 into CiscoTestAutomation:master Mar 13, 2024
4 checks passed
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.

3 participants