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 Enterprise SONIC LLDP Interface module #308

Merged
merged 12 commits into from
May 28, 2024

Conversation

m-nazeer
Copy link
Contributor

@m-nazeer m-nazeer commented Oct 31, 2023

SUMMARY

LLDP_INTERFACES Module implementation

GitHub Issues

N/A

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

sonic_lldp_interfaces

OUTPUT

Regression Test HTML report:
lldp_interface_log.zip

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

@m-nazeer m-nazeer changed the title Lldp new Add Enterprise SONIC LLDP Interface module Oct 31, 2023
@ArunSaravananBalachandran ArunSaravananBalachandran added the new_resource_module This pull request adds a new resource module label Nov 7, 2023
@jeff-yin jeff-yin added this to the v2.4.0 milestone Nov 29, 2023
@kerry-meyer
Copy link
Collaborator

Closing and re-opening to clear "failure" for a missing fragment file.

@kerry-meyer kerry-meyer reopened this Jan 25, 2024
@kerry-meyer
Copy link
Collaborator

To proceed with the code review for this resource module, please push changes for the following items:

  • Re-sync your fork and branch to pull in updates from the "main" branch that are the probable cause of the flagged sanity failures for python3.10, 3.11, and 3.12 with the ansible-core "devel" version.
  • Add Unit Test coverage. Use the UT implementation for the DHCP Relay module as an example of how to do this.

While you are working on the UT changes, we may go ahead with checking the changes that are in the current change set and posting an initial set of comments/questions (if any) for the current code. The PR can not be approved or merged, however, until the items described above have been completed.

@mingjunzhang2019 mingjunzhang2019 self-requested a review March 21, 2024 18:30
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.

I'm changing the state on this PR to "Request changes" to make it easier to see from the top level PR view.

Please address the comments posted by Mingjun Zhang either with the corresponding modifications in the change set or with response comments (for cases in which you would like clarification of the posted comment or do not agree with it).

@m-nazeer
Copy link
Contributor Author

to

Hi Kerry. I am working on addressing the review comments for overridden and replaced logic. I will push the code changes once done

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 incremental changes to address Mingjun's review comments.

I have marked some of the "Issues" posted by Mingjun as "Resolved", but some are not, and I have posted a couple of additional comments.

  • Please address the remaining open issues and post updated test results. In the posted test results, please format the summary output in a way that displays the 'before' and 'after' output without truncation. (Landscape, scaled as needed.)
  • Please ensure that overridden/replaced test cases are causing deletion of some attributes within existing interface configurations. (This may already be the case, but it's not easy to see from the currently posted test results or by 'walking' the test list.)
  • Please fix the UT failures and sanity failures.
  • Please re-sync the PR branch to pick up changes from the 'main' branch and to resolve any conflicts that currently exist for shared files.

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 code for the current proposed change set, the test case coverage, and all test results look good.

Approved.

Thank you for providing the LLDP Interfaces resource module for the Dell enterprise_sonic resource module collection.

@kerry-meyer kerry-meyer merged commit 64c03ba into ansible-collections:main May 28, 2024
13 checks passed
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.

5 participants