-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Sync Mode drift detection #70811
Sync Mode drift detection #70811
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #70811 +/- ##
=======================================
Coverage 80.01% 80.02%
=======================================
Files 6505 6505
Lines 290769 290764 -5
Branches 50112 50118 +6
=======================================
Hits 232670 232670
+ Misses 57662 57657 -5
Partials 437 437 |
c01685a
to
45f1808
Compare
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.
I think I am missing something here.
Are you saying that we have options that are set on disk and also on the DB to a value that is not the default value for the option?
This seems something that should not happen as we do not allow an option to be updated on DB if it is set on disk.
This could only have happened if the option was already set to a value on the DB (different than the default) and then set on disk to something different again.
But in that case it means the option cannot be updated with any channel, we should not hide this scenario which is broken.
We have options that are set on disk. In the db, they are set to their default value. I don't think this behavior is broken, as FLAG_PRIORTIZE_DISK seems to do exactly as said, take the disk value if it exists, otherwise read from the db. Previously, when My change just checks if an option is already set on disk, if it is, don't write it as drift. |
This reverts commit ade7324.
0ddc36b
to
b6c29b8
Compare
presenter_delegator.drift(opt.name, "") | ||
drift_found = True | ||
if ( | ||
options.can_update( |
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.
if an option is passed into configoptions, and it is set on disk, we check for it here (when we call attempt_update on line 244) https://github.com/getsentry/sentry/blob/fix/options-on-disk/src/sentry/options/manager.py#L188-L200
otherwise, if it is set on disk but not defined in the automator, since options.get() and options.isset() will still return true,(even if the options DNE in the DB) we again check to see if the option is on disk. if it is, we continue.
In Sync mode, we do not want options that are not called in configoptions but
set_on_disk
to show up as drift.Some options have the
prioritize_disk
flag and have values on the db and the disk.configoptions should only update these options if they are not set_on_disk. Right now, if it is set_on_disk but also set in the db, we write it as drift. This PR adds a check for that.