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.

Sign up for GitHub

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

Jump to bottom

Add support for new Port SI parameters in PortsOA #2929

Merged
merged 11 commits into from
Dec 1, 2023
Merged

Add support for new Port SI parameters in PortsOA #2929

merged 11 commits into from
Dec 1, 2023

Conversation

tshalvi
Copy link
Contributor

@tshalvi tshalvi commented Oct 12, 2023 •
edited
Loading

What I did
I added six new SI parameters to the relevant locations in SWSS.

Why I did it
I made these additions to support the new SI parameters recently added to APP_DB.

How I verified it
I verified it using the existing unit tests, and I will be adding new unit tests in the near future.

@tshalvi tshalvi mentioned this pull request Oct 12, 2023
Merged
@tshalvi tshalvi changed the title Independent Module - swss code to support port SI configuration per speed Independent Module - New SI Parameters Oct 12, 2023
@tshalvi tshalvi marked this pull request as ready for review October 17, 2023 09:48
@tshalvi tshalvi requested a review from prsunny as a code owner October 17, 2023 09:48
@tshalvi tshalvi changed the title Independent Module - New SI Parameters Add support for new Port SI parameters in PortsOA Oct 17, 2023
prgeor
prgeor reviewed Oct 19, 2023
View reviewed changes
@tshalvi tshalvi requested a review from prgeor October 23, 2023 15:31
@liat-grozovik
Copy link
Collaborator

@prgeor kindly reminder to review this PR.

prgeor
prgeor approved these changes Nov 20, 2023
View reviewed changes
Copy link
Contributor

@prgeor prgeor left a comment

Choose a reason for hiding this comment

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

@tshalvi LGTM. Will be good to update the PR description where you mentioned adding the logs for debug which is now removed in your latest update

@prsunny
Copy link
Collaborator

prsunny commented Nov 22, 2023

@tshalvi , @prgeor , could you add unit test for this PR?

@tshalvi
Copy link
Contributor Author

tshalvi commented Nov 27, 2023

@tshalvi , @prgeor , could you add unit test for this PR?

According to the checker, it looks like my code changes are 100% covered. Could you please clarify which additional tests are required in this case?

@prsunny
Copy link
Collaborator

prsunny commented Nov 28, 2023

@tshalvi , @prgeor , could you add unit test for this PR?

According to the checker, it looks like my code changes are 100% covered. Could you please clarify which additional tests are required in this case?

there is portsorch unit test which can cover the new sections

@prsunny prsunny merged commit 8dc0a85 into sonic-net:master Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prsunny prsunny prsunny approved these changes

@keboliu keboliu keboliu approved these changes

@prgeor prgeor prgeor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tshalvi @liat-grozovik @prsunny @keboliu @prgeor