Fix for configoptions, remove check for isset() (#71089)

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.

---------

Co-authored-by: Mark Story <mark@mark-story.com>

src/sentry/options/manager.py

@@ -251,7 +251,9 @@ class OptionsManager:
 
     def isset(self, key: str) -> bool:
         """
-        Check if a key has been set to a value and not inheriting from its default.
+        Check if a key is set on the local cache, network cache, or db in that order.
+        Keep in mind that if an option is deleted, any new calls to options.get()
+        will repopulate the cache, resulting in this method to return true.
         """
         opt = self.lookup_key(key)
 

src/sentry/runner/commands/configoptions.py

@@ -259,29 +259,31 @@ def sync(ctx: click.Context) -> None:
                     presenter_delegator.flush()
                     raise
             else:
-                if options.isset(opt.name):
-                    if options.get_last_update_channel(opt.name) == options.UpdateChannel.AUTOMATOR:
-                        if not dry_run:
-                            try:
-                                options.delete(opt.name)
-                            except Exception:
-                                metrics.incr(
-                                    "options_automator.run",
-                                    amount=2,
-                                    tags={"status": "update_failed"},
-                                    sample_rate=1.0,
-                                )
-                                presenter_delegator.flush()
-                                raise
-                        presenter_delegator.unset(opt.name)
-                    else:
-                        # If an option is set on disk, but not passed into configoptions,
-                        # we can safely continue.
-                        if options.is_set_on_disk(opt.name):
-                            continue
-                        else:
-                            presenter_delegator.drift(opt.name, options.get(opt.name))
-                            drift_found = True
+                last_updated = options.get_last_update_channel(opt.name)
+
+                # for options that are set on disk, last_updated should be None
+                if last_updated == options.UpdateChannel.AUTOMATOR:
+                    if not dry_run:
+                        try:
+                            options.delete(opt.name)
+                        except Exception:
+                            metrics.incr(
+                                "options_automator.run",
+                                amount=2,
+                                tags={"status": "update_failed"},
+                                sample_rate=1.0,
+                            )
+                            presenter_delegator.flush()
+                            raise
+                    presenter_delegator.unset(opt.name)
+                elif last_updated == options.UpdateChannel.CLI:
+                    presenter_delegator.drift(opt.name, options.get(opt.name))
+                    drift_found = True
+                elif last_updated == options.UpdateChannel.UNKNOWN:
+                    presenter_delegator.drift(opt.name, options.get(opt.name))
+                    drift_found = True
+                else:
+                    continue
 
     if invalid_options:
         status = "update_failed"

tests/sentry/runner/commands/test_configoptions.py

@@ -208,6 +208,48 @@ class ConfigOptionsTest(CliTestCase):
 
         assert ConsolePresenter.ERROR_MSG % ("set_on_disk_option", "option_on_disk") in rv.output
 
+    def test_sync_unset_options(self):
+
+        # test options set on disk with and without prioritize disk, tracked
+        # and not tracked
+        # test options set on db, verify that untracked options are properly deleted
+
+        options.delete("drifted_option")
+
+        rv = self.invoke(
+            "-f",
+            "tests/sentry/runner/commands/unsetsync.yaml",
+            "sync",
+        )
+        assert rv.exit_code == 0, rv.output
+        expected_output = "\n".join(
+            [
+                ConsolePresenter.CHANNEL_UPDATE_MSG % "change_channel_option",
+                ConsolePresenter.SET_MSG % ("map_option", {"a": 1, "b": 2}),
+                ConsolePresenter.SET_MSG % ("list_option", [1, 2]),
+                ConsolePresenter.UNSET_MSG % "str_option",
+                ConsolePresenter.UNSET_MSG % "to_unset_option",
+            ]
+        )
+
+        assert expected_output in rv.output
+
+        assert options.get("int_option") == 20
+        assert options.get("str_option") == "blabla"
+        assert options.get("map_option") == {
+            "a": 1,
+            "b": 2,
+        }
+        assert options.get("list_option") == [1, 2]
+
+        # assert there's no drift after unsetting
+        rv = self.invoke(
+            "-f",
+            "tests/sentry/runner/commands/unsetsync.yaml",
+            "sync",
+        )
+        assert rv.exit_code == 0, rv.output
+
     def test_bad_patch(self):
         rv = self.invoke(
             "--file=tests/sentry/runner/commands/badpatch.yaml",

tests/sentry/runner/commands/unsetsync.yaml

@@ -0,0 +1,11 @@
+options:
+  map_option:
+    a: 1
+    b: 2
+  list_option:
+    - 1
+    - 2
+  change_channel_option:
+    - 5
+    - 6
+    - 7