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

Ospf area bugfix #466

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Conversation

stalabi1
Copy link
Collaborator

@stalabi1 stalabi1 commented Oct 10, 2024

SUMMARY

Fixed bug in delete method where there was no condition handling the case of no requests to delete.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

sonic_ospf_area

OUTPUT

failing report:
regression-2024-10-10-13-44-49.html.pdf

Passing report after change:
regression-2024-10-10-14-42-02.html.pdf

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 the previous 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

@stalabi1 stalabi1 added the bug Something isn't working label Oct 10, 2024
# clearing some of the settings in stub but not all
# commands has to be a subset of have and whatever in it is translated into requests
return False, requests, False

Copy link
Collaborator

@kerry-meyer kerry-meyer Oct 10, 2024

Choose a reason for hiding this comment

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

Actually, none of the "settings" were getting cleared for the problem case, so this comment isn't completely accurate.

Also, the wording of the existing comment was confusing.

Suggested re-wording for this comment:

Not all stub configuration is being deleted. (It is also possible that none of the stub options requested for deletion match any current configuration. In that case, no stub configuration is being deleted.)

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 code change and corresponding test results look good.

I am posting only a request for re-wording of the comment related to the modified section of code.

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 the incremental edit of the comment for the affected section of code.

All current proposed changes and test results look good.

Thanks for finding and fixing this bug.

Approved.

@kerry-meyer kerry-meyer merged commit c630266 into ansible-collections:main Oct 10, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants