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

Ansible collections for USGv6R1 related features #374

Merged
merged 13 commits into from
Jun 13, 2024

Conversation

pprakashsamy
Copy link
Contributor

@pprakashsamy pprakashsamy commented May 3, 2024

SUMMARY

  1. Addition of USGv6R1 feature related attributes in Enterprise SONiC resource module:
    Data definitions in 'argspec'.
    Implementation of 'facts' to gather facts for the module.
    Implementation of 'config' to apply configurations for merged and deleted states.
  2. Definition of regression test cases to verify correct functionality of USGv6R1 related attributes.

ISSUE TYPE

  • Feature Pull Request

COMPONENT NAME
sonic_l3_interfaces

OUTPUT
Test Result:
Regression_USGv6R1_Ansible.pdf

Latest:
regression-2024-06-10-15-57-23.zip

DEPENDANT PR:
ansible-network/resource_module_models#255

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@kerry-meyer
Copy link
Collaborator

kerry-meyer commented Jun 10, 2024

Thank you for making the incremental changes to address the pending review comments.

I triggered a run of the sanity tests after the latest pushes to the PR branch and there are some failures that need to be addressed before this PR can be approved and merged:

  • Please fix the UT failure for the l3_interfaces module (See the "Details" link for "Code coverage / Code coverage | Python 3.10 (pull_request) Failing after 33s ".)

  • Please fix the sanity failures (related to a changed documentation block in the l3_interfaces "module" file).


Also, the most recently posted regression test summary output is not readable. Please generate a new one that is a readable ".pdf" file (by printing the verbose '.html' regression test summary file for the regression run to a ".pdf" file).

pprakashsamy and others added 2 commits June 11, 2024 12:11
Co-authored-by: Arun Saravanan Balachandran <[email protected]>
Co-authored-by: Arun Saravanan Balachandran <[email protected]>
Copy link
Collaborator

@kerry-meyer kerry-meyer left a comment

Choose a reason for hiding this comment

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

The proposed change set and corresponding test results look good.

I am requesting two very minor documentation changes (single line changes). No regression test re-run is needed after pushing these changes.

plugins/modules/sonic_l3_interfaces.py Outdated Show resolved Hide resolved
ipv6:
addresses:
- address: 85::/64
eui64: True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since deletion of the address causes deletion of the eui64 bool regardless of whether the bool deletion is explicitly requested, the user doesn't need to specify "eui64: True" to do the deletion for this case. It would provide better guidance to our users to omit this line in the example.

Copy link
Collaborator

@kerry-meyer kerry-meyer 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 making the final set of requested incremental changes.

The current set of proposed changes and corresponding test results look good.

Thank you very much for providing the code changes for support of the USGv6R1 related changes.

Approved.

@kerry-meyer kerry-meyer merged commit 0793688 into ansible-collections:main Jun 13, 2024
16 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