Browse Source

ref(crons): Clarify that `schedule_type` is ALWAYS set (#57054)

We used to have logic that implied that there is a case where
`schedule_type` was not set and that it was "implicitly" `crontab`. That
is not the case. In sentry.io production right now

```sql
select
  count(*)
from
  sentry_monitor
where
  (config :: json ->> 'schedule_type') :: int not in (1, 2);
```

This is `0`. We can be explicit that this is always set.

Our validator is already requiring it here


https://github.com/getsentry/sentry/blob/b06950b2aab599c49d8ee71f5adf51fb2ff1a378/src/sentry/monitors/validators.py#L180-L181
Evan Purkhiser 1 year ago
parent
commit
b8857d0c32
2 changed files with 26 additions and 28 deletions
  1. 5 4
      src/sentry/monitors/models.py
  2. 21 24
      tests/sentry/monitors/test_models.py

+ 5 - 4
src/sentry/monitors/models.py

@@ -61,8 +61,9 @@ MONITOR_CONFIG = {
         "failure_issue_threshold": {"type": ["integer", "null"]},
         "recovery_threshold": {"type": ["integer", "null"]},
     },
-    # TODO(davidenwang): Old monitors may not have timezone or schedule_type, these should be added here once we've cleaned up old data
-    "required": ["checkin_margin", "max_runtime", "schedule"],
+    # TODO(davidenwang): Old monitors may not have timezone, it should be added
+    # here once we've cleaned up old data
+    "required": ["checkin_margin", "max_runtime", "schedule_type", "schedule"],
     "additionalProperties": False,
 }
 
@@ -216,7 +217,7 @@ class ScheduleType:
 
     @classmethod
     def as_choices(cls):
-        return ((cls.UNKNOWN, "unknown"), (cls.CRONTAB, "crontab"), (cls.INTERVAL, "interval"))
+        return ((cls.CRONTAB, "crontab"), (cls.INTERVAL, "interval"))
 
     @classmethod
     def get_name(cls, value):
@@ -264,7 +265,7 @@ class Monitor(Model):
         return super().save(*args, **kwargs)
 
     def get_schedule_type_display(self):
-        return ScheduleType.get_name(self.config.get("schedule_type", ScheduleType.CRONTAB))
+        return ScheduleType.get_name(self.config["schedule_type"])
 
     def get_audit_log_data(self):
         return {"name": self.name, "type": self.type, "status": self.status, "config": self.config}

+ 21 - 24
tests/sentry/monitors/test_models.py

@@ -20,9 +20,14 @@ from sentry.testutils.silo import region_silo_test
 
 @region_silo_test(stable=True)
 class MonitorTestCase(TestCase):
-    def test_next_run_crontab_implicit(self):
+    def test_next_run_crontab(self):
         ts = datetime(2019, 1, 1, 1, 10, 20, tzinfo=timezone.utc)
-        monitor = Monitor(config={"schedule": "* * * * *"})
+        monitor = Monitor(
+            config={
+                "schedule_type": ScheduleType.CRONTAB,
+                "schedule": "* * * * *",
+            }
+        )
         monitor_environment = MonitorEnvironment(monitor=monitor, last_checkin=ts)
 
         # XXX: Seconds are removed as we clamp to the minute
@@ -41,23 +46,14 @@ class MonitorTestCase(TestCase):
             2019, 1, 1, 1, 16, tzinfo=timezone.utc
         )
 
-    def test_next_run_latest_crontab_implicit(self):
-        ts = datetime(2019, 1, 1, 1, 10, 20, tzinfo=timezone.utc)
-        monitor = Monitor(config={"schedule": "* * * * *", "checkin_margin": 5})
-        monitor_environment = MonitorEnvironment(monitor=monitor, last_checkin=ts)
-
-        # XXX: Seconds are removed as we clamp to the minute
-        assert monitor_environment.monitor.get_next_expected_checkin(ts) == datetime(
-            2019, 1, 1, 1, 11, tzinfo=timezone.utc
-        )
-        assert monitor_environment.monitor.get_next_expected_checkin_latest(ts) == datetime(
-            2019, 1, 1, 1, 16, tzinfo=timezone.utc
-        )
-
-    def test_next_run_crontab_explicit(self):
+    def test_next_run_latest_crontab_with_margin(self):
         ts = datetime(2019, 1, 1, 1, 10, 20, tzinfo=timezone.utc)
         monitor = Monitor(
-            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+            config={
+                "schedule_type": ScheduleType.CRONTAB,
+                "schedule": "* * * * *",
+                "checkin_margin": 5,
+            }
         )
         monitor_environment = MonitorEnvironment(monitor=monitor, last_checkin=ts)
 
@@ -65,18 +61,16 @@ class MonitorTestCase(TestCase):
         assert monitor_environment.monitor.get_next_expected_checkin(ts) == datetime(
             2019, 1, 1, 1, 11, tzinfo=timezone.utc
         )
-
-        monitor.config["schedule"] = "*/5 * * * *"
-        assert monitor_environment.monitor.get_next_expected_checkin(ts) == datetime(
-            2019, 1, 1, 1, 15, tzinfo=timezone.utc
+        assert monitor_environment.monitor.get_next_expected_checkin_latest(ts) == datetime(
+            2019, 1, 1, 1, 16, tzinfo=timezone.utc
         )
 
-    def test_next_run_crontab_explicit_timezone(self):
+    def test_next_run_crontab_with_timezone(self):
         ts = datetime(2019, 1, 1, 1, 10, 20, tzinfo=timezone.utc)
         monitor = Monitor(
             config={
-                "schedule": "0 12 * * *",
                 "schedule_type": ScheduleType.CRONTAB,
+                "schedule": "0 12 * * *",
                 "timezone": "UTC",
             },
         )
@@ -97,7 +91,10 @@ class MonitorTestCase(TestCase):
     def test_next_run_interval(self):
         ts = datetime(2019, 1, 1, 1, 10, 20, tzinfo=timezone.utc)
         monitor = Monitor(
-            config={"schedule": [1, "month"], "schedule_type": ScheduleType.INTERVAL},
+            config={
+                "schedule": [1, "month"],
+                "schedule_type": ScheduleType.INTERVAL,
+            },
         )
         monitor_environment = MonitorEnvironment(monitor=monitor, last_checkin=ts)