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 support for Versatile hash feature #401

Merged
merged 9 commits into from
Jul 25, 2024

Conversation

ohu1
Copy link
Contributor

@ohu1 ohu1 commented Jun 21, 2024

SUMMARY

Ansible support for Versatile Hash feature

GitHub Issues

"N/A".

GitHub Issue #
ISSUE TYPE

N/A

COMPONENT NAME

sonic-system (Update existing module)

ADDITIONAL INFORMATION

N/A
`

Checklist:
  • [ x] I have performed a self-review of my own code to ensure there are no formatting, linting, or security issues
  • [x ] I have verified that new and existing unit tests pass locally with my changes
  • [ x] I have not allowed coverage numbers to degenerate
  • [x ] I have maintained at least 90% code coverage
  • [ x] I have commented my code, particularly in hard-to-understand areas
  • [x ] I have made corresponding changes to the documentation
  • [x ] I have added tests that prove my fix is effective or that my feature works
  • [ x] I have maintained backward compatibility or have provided any relevant "breaking_changes" descriptions in a "fragment" file in the "changelogs/fragments" directory of this repository.
  • [ x] I have provided a summary for this PR in valid "fragment" file format in the "changelogs/fragments" directory of this repository branch. Reference : Ansible Change Log Document
How Has This Been Tested?

Regression test with updated testcases.

regression-2024-06-28-12-07-00.html.gz

@ohu1 ohu1 changed the title Ansible support for Versatile hash feature WIP: Ansible support for Versatile hash feature Jun 21, 2024
@ohu1 ohu1 changed the title WIP: Ansible support for Versatile hash feature Ansible support for Versatile hash feature Jun 21, 2024
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 implementing Ansible support for the Versatile hash feature.

I have posted a few file and inline comments to address for this PR. In addition, please fix the UT and sanity errors that are flagged for this PR.

Note: You can pre-run the sanity tests (including UT) before pushing changes to the PR branch using the instructions at:

https://github.com/ansible-collections/dellemc.enterprise_sonic/wiki/Running-Unit-Tests-and-Coverage-Report-Generation

@ohu1
Copy link
Contributor Author

ohu1 commented Jun 28, 2024

attached new regression result: pass

@ohu1 ohu1 requested a review from kerry-meyer June 28, 2024 00:43
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 incremental changes to address the first pass review comments.

There are still some problems to address.

I have posted some specific comments for these.

An additional comment: Please pre-run the sanity tests using the instructions at "https://github.com/ansible-collections/dellemc.enterprise_sonic/wiki/Running-Unit-Tests-and-Coverage-Report-Generation" and fix any remaining sanity errors before pushing the new changes to this PR.

plugins/modules/sonic_system.py Outdated Show resolved Hide resolved
changelogs/fragments/401-versatile-hash.yaml Outdated Show resolved Hide resolved
plugins/module_utils/network/sonic/facts/system/system.py Outdated Show resolved Hide resolved
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 additional changes to get rid of the "empty" value display in facts gathering. The regression result output looks fine now.

There are still some sanity checks failing.

I am posting comment for one (a hyphen in a list line that is apparently causing YAML parsing problems.)

The first 3 of the following 4 errors are easy to find and fix:

ERROR: Found 4 pylint issue(s) which need to be resolved: ERROR: plugins/module_utils/network/sonic/config/system/system.py:62:8: undefined-variable: Undefined variable 'new_config' ERROR: plugins/module_utils/network/sonic/config/system/system.py:401:15: singleton-comparison: Comparison 'load_share_hash_algo != None' should be 'load_share_hash_algo is not None' ERROR: plugins/module_utils/network/sonic/config/system/system.py:465:23: trailing-whitespace: Trailing whitespace ERROR: plugins/module_utils/network/sonic/config/system/system.py:468:0: trailing-newlines: Trailing newlines

Please push changes to address the items described here and in the new comment.

plugins/modules/sonic_system.py Outdated Show resolved Hide resolved
@ohu1
Copy link
Contributor Author

ohu1 commented Jul 1, 2024

Thank you for making the additional changes to get rid of the "empty" value display in facts gathering. The regression result output looks fine now.

There are still some sanity checks failing.

I am posting comment for one (a hyphen in a list line that is apparently causing YAML parsing problems.)

The first 3 of the following 4 errors are easy to find and fix:

ERROR: Found 4 pylint issue(s) which need to be resolved: ERROR: plugins/module_utils/network/sonic/config/system/system.py:62:8: undefined-variable: Undefined variable 'new_config' ERROR: plugins/module_utils/network/sonic/config/system/system.py:401:15: singleton-comparison: Comparison 'load_share_hash_algo != None' should be 'load_share_hash_algo is not None' ERROR: plugins/module_utils/network/sonic/config/system/system.py:465:23: trailing-whitespace: Trailing whitespace ERROR: plugins/module_utils/network/sonic/config/system/system.py:468:0: trailing-newlines: Trailing newlines

Please push changes to address the items described here and in the new comment.

The above errors are fixed now.

@ohu1 ohu1 requested a review from kerry-meyer July 1, 2024 21:00
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.

All issues have been resolved, and sanity tests are passing with most recent change set.

Approved.

@kerry-meyer kerry-meyer merged commit 4bda42f into ansible-collections:main Jul 25, 2024
15 checks passed
@kerry-meyer kerry-meyer mentioned this pull request Sep 5, 2024
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