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 RoCE module #363

Merged
merged 8 commits into from
May 29, 2024
Merged

Add RoCE module #363

merged 8 commits into from
May 29, 2024

Conversation

stalabi1
Copy link
Collaborator

@stalabi1 stalabi1 commented Apr 6, 2024

SUMMARY

I added the RoCE module.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

sonic_roce

OUTPUT

regression-2024-05-15-15-22-53.html.pdf
regression-2024-05-15-15-08-43.html.pdf
diff_output.log
facts_gathering.log
show_running.txt

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
  • 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.

Copy link
Collaborator

@xhan-dell xhan-dell left a comment

Choose a reason for hiding this comment

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

Looks pretty good, a few comments

plugins/module_utils/network/sonic/facts/roce/roce.py Outdated Show resolved Hide resolved
plugins/module_utils/network/sonic/config/roce/roce.py Outdated Show resolved Hide resolved
plugins/modules/sonic_roce.py Show resolved Hide resolved
plugins/module_utils/network/sonic/config/roce/roce.py Outdated Show resolved Hide resolved
@stalabi1
Copy link
Collaborator Author

@xhan-dell Thanks for the comments. The PR isn't ready but I just wanted to create a draft for Kerry to easily see my code as we are debugging the reboot issue.

@stalabi1 stalabi1 closed this Apr 11, 2024
@stalabi1 stalabi1 reopened this May 15, 2024
@stalabi1 stalabi1 added the new_resource_module This pull request adds a new resource module label May 15, 2024
@stalabi1 stalabi1 marked this pull request as ready for review May 15, 2024 22:45
@stalabi1 stalabi1 requested a review from kerry-meyer as a code owner May 15, 2024 22:45
@stalabi1 stalabi1 changed the title Initial commit for RoCE Add RoCE module May 15, 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.

Thanks for all of your hard work to implement this difficult new configuration case.

The proposed code, new tests, and test results look good.

My only comments are for minor wording changes in documentation and comments.

plugins/modules/sonic_roce.py Outdated Show resolved Hide resolved
plugins/modules/sonic_roce.py Outdated Show resolved Hide resolved
plugins/module_utils/network/sonic/sonic.py Outdated Show resolved Hide resolved
plugins/modules/sonic_qos_buffer.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.

Thanks for making these incremental revisions.

All issues are resolved now. The current proposed code, test coverage, and test results look good.

Approved.

@kerry-meyer kerry-meyer merged commit 37fddc3 into ansible-collections:main May 29, 2024
13 checks passed
@stalabi1 stalabi1 deleted the roce branch July 10, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new_resource_module This pull request adds a new resource module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants