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_eigrp_router_id API to verify.py ios/iosxe eigrp protocol (my old contribution) supporting EIGRPv4 and EIGRPv6 for IOS/IOSXE #158

Merged
merged 26 commits into from
Apr 8, 2024

Conversation

cherifimehdi
Copy link
Contributor

Hello,

I added verify_eigrp_router_id API to verify.py (my old contribution) for ios/iosxe eigrp protocol.

I added the tests at pkgs/sdk-pkg/src/genie/libs/sdk/apis/tests/iosxe/eigrp/verify

I added the changelog_add_verify_eigrp_router_id_202421031402 file at pkgs/sdk-pkg/changelog folder

Thank you for reviewing my contribution

add changelog_add_verify_eigrp_router_id_202421031402
Add new verify.py containing new API verify_eigrp_router_id
Add test_api_verify_eigrp_router_id.py
@cherifimehdi
Copy link
Contributor Author

Hello @omehrabi @lsheikal thank you for reviewing my PR.
Thanks

@cherifimehdi
Copy link
Contributor Author

Hi @lsheikal @omehrabi please can you review and merge my PR since it is an addition to my previous contribution (verifi.py for iosxe eigrp) and I followed same logic based on reviewers from my previous PR @ThomasJRyan and @Lelor.
Thank you

@omehrabi
Copy link
Contributor

there is a change in unicon.plugin which will break the unittest for genielibs please update tests after 24.3 release

@cherifimehdi
Copy link
Contributor Author

Hi @omehrabi I fixed the problem with the update tests after 24.3 release

@cherifimehdi
Copy link
Contributor Author

there is a change in unicon.plugin which will break the unittest for genielibs please update tests after 24.3 release

Thank you, Hi @omehrabi , the problem with the update is fixed

cherifimehdi and others added 4 commits March 28, 2024 16:45
Co-authored-by: Lukeman Hakkim <84532451+lsheikal@users.noreply.github.com>
Add new verify.py with the asked changes
Co-authored-by: Lukeman Hakkim <84532451+lsheikal@users.noreply.github.com>
@cherifimehdi
Copy link
Contributor Author

Hi @lsheikal why there is problem with test?

@lsheikal
Copy link
Contributor

Hi @lsheikal why there is problem with test?

pull the latest dev and merge with your branch. And address my comments.

Fixed

Co-authored-by: Lukeman Hakkim <84532451+lsheikal@users.noreply.github.com>
Copy link
Contributor Author

@cherifimehdi cherifimehdi left a comment

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

@cherifimehdi cherifimehdi left a comment

Choose a reason for hiding this comment

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

I fixed this

Copy link
Contributor Author

@cherifimehdi cherifimehdi left a comment

Choose a reason for hiding this comment

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

Change applied

@cherifimehdi
Copy link
Contributor Author

Hi @lsheikal same problem, I don't know way test failed

@cherifimehdi
Copy link
Contributor Author

Hi @omehrabi @lsheikal I think problem with test fixed

@cherifimehdi
Copy link
Contributor Author

Hi @omehrabi @lsheikal are the corrections I made OK or There are modifications to do? Thank you

@lsheikal
Copy link
Contributor

lsheikal commented Apr 1, 2024

Hi @omehrabi @lsheikal are the corrections I made OK or There are modifications to do? Thank you

you still have not addressed the comments, i can still see the parameter AS_N in upper case, which violates pep8 standards. change it to a meaningful parameter name in lowercase.

@cherifimehdi
Copy link
Contributor Author

Hi @omehrabi @lsheikal are the corrections I made OK or There are modifications to do? Thank you

you still have not addressed the comments, i can still see the parameter AS_N in upper case, which violates pep8 standards. change it to a meaningful parameter name in lowercase.

Hi @omehrabi @lsheikal thank you. should I modify 'AS_N' just for this API or for all previous APIs merged previously

@lsheikal
Copy link
Contributor

lsheikal commented Apr 1, 2024

Hi @omehrabi @lsheikal are the corrections I made OK or There are modifications to do? Thank you

you still have not addressed the comments, i can still see the parameter AS_N in upper case, which violates pep8 standards. change it to a meaningful parameter name in lowercase.

Hi @omehrabi @lsheikal thank you. should I modify 'AS_N' just for this API or for all previous APIs merged previously

lets modify this parameter for this api. And kindly adhere to the pep8 standards hereafter.

Change AS_N to auto_sys
@cherifimehdi
Copy link
Contributor Author

cherifimehdi commented Apr 1, 2024

Hi @omehrabi @lsheikal are the corrections I made OK or There are modifications to do? Thank you

you still have not addressed the comments, i can still see the parameter AS_N in upper case, which violates pep8 standards. change it to a meaningful parameter name in lowercase.

Hi @omehrabi @lsheikal thank you. should I modify 'AS_N' just for this API or for all previous APIs merged previously

lets modify this parameter for this api. And kindly adhere to the pep8 standards hereafter.

Hi @omehrabi @lsheikal I changed 'AS_N' to 'auto_sys' in the API as requested

@cherifimehdi
Copy link
Contributor Author

Hi @lsheikal, are the changes OK or are there more to do besides "AS_N" (I change it as requested)? THANKS

Copy link
Contributor

@omehrabi omehrabi left a comment

Choose a reason for hiding this comment

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

there have been a change in unicon please update to the latest version and record the mock data

…verify_eigrp_router_id/mock_data/iosxe/mock_data.yaml

Delete old one
Add new mock_data.yaml
…verify_eigrp_router_id/test_api_verify_eigrp_router_id.py

Delete old one
Add new test_api_verify_eigrp_router_id.py
@cherifimehdi
Copy link
Contributor Author

cherifimehdi commented Apr 2, 2024

Hi @omehrabi I updated to the latest version and I updated mock data and tests. Thank you
Is there any other change to make or it is ok?

@cherifimehdi
Copy link
Contributor Author

Hi @omehrabi @lsheikal Is there any other change to make or it is ok? Thanks

@cherifimehdi
Copy link
Contributor Author

Hi @omehrabi Is there any other change to make or it is ok? Thanks

@cherifimehdi
Copy link
Contributor Author

Hi @omehrabi Is there any other change to make or it is ok? Thanks

@omehrabi omehrabi merged commit 85616ca into CiscoTestAutomation:master Apr 8, 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