Fix for configoptions, remove check for isset()

[kneeyo1]

We only need check the last_updated_by,

if the option is already unset (removed from db), last_updated_by will be None. All other channels we can write as drift.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.91%. Comparing base (2cf3c5e) to head (410c6a4).
Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #71089   +/-   ##
=======================================
  Coverage   77.91%   77.91%           
=======================================
  Files        6528     6528           
  Lines      290912   290912           
  Branches    50340    50340           
=======================================
  Hits       226654   226654           
+ Misses      58036    58035    -1     
- Partials     6222     6223    +1     

Files	Coverage Δ
src/sentry/options/manager.py	97.42% <ø> (ø)

... and 4 files with indirect coverage changes

[markstory]

We could also change isset() so that it behaves correctly. Currently the docstring for isset claims

Check if a key has been set to a value and not inheriting from its default.

Which doesn't match its current behavior. We could have isset compare the value from the store to the option default. If the store value and default are the same, the option is not 'set'.

[kneeyo1]

I originally wanted to do that but serveral other files and methods use issset(), worried I could break their functionality

[kneeyo1]

The other issue is that if this is what we use to compare options, configoptions would never actually remove options from the DB if the DB value is just the default value.

[markstory]

I originally wanted to do that but serveral other files and methods use issset(), worried I could break their functionality

We could review those callsites and see if their behaviour would change.

The other issue is that if this is what we use to compare options, configoptions would never actually remove options from the DB if the DB value is just the default value.

If the value is set in the store and equal to the default, wouldn't we still be able to check the option's update channel?

[kneeyo1]

Spoke with @markstory and decided that fixing isset() is the better course of action.

[markstory]

You could put this yaml file with other test data in fixtures. It could be helpful to have an options directory for this and other test fixture files.

[markstory]

Looks good to me. Options that were automated and have been removed from the options data will now be removed if the option's last update was through automator. other states that aren't default/disk values will result in drift 👍