-
Notifications
You must be signed in to change notification settings - Fork 53
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
🐛 fix: do not silently drop finalizer updates when status is also updated #1283
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1283 +/- ##
==========================================
+ Coverage 76.49% 76.51% +0.01%
==========================================
Files 39 39
Lines 2361 2363 +2
==========================================
+ Hits 1806 1808 +2
Misses 389 389
Partials 166 166
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
codecov/patch
test seems to be failing because I moved the check for unexpected changes.
Fwiw there were no tests added to confirm this was a problem before(which is fine coz we didn't know it was a problem), but there's no tests now to confirm on paper it is still not a problem
@anik120 not sure we want to test a panic, also not sure how to get a unit test to modify fields while we're in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
34821fa
This is a fix for a bug that @anik120 stumbled upon in catalogd while working on operator-framework/catalogd#379.
Wanted to fix it here as well while we're thinking about it.
Description
Reviewer Checklist