fix(crons): Initial removal of status from Monitor object #47910
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #47910 +/- ##
==========================================
- Coverage 80.86% 80.86% -0.01%
==========================================
Files 4769 4769
Lines 201565 201543 -22
Branches 11557 11557
==========================================
- Hits 162990 162970 -20
+ Misses 38319 38317 -2
Partials 256 256
|
| @@ -111,6 +112,8 @@ def put(self, request: Request, organization, project, monitor) -> Response: | |||
|
|
|||
| if params: | |||
| monitor.update(**params) | |||
| if params.get("status"): | |||
| MonitorEnvironment.objects.filter(monitor=monitor).update(status=params["status"]) | |||
There was a problem hiding this comment.
this will fix a sort bug by marking the MonitorEnvironment as DISABLED
There was a problem hiding this comment.
Are we sure we want this? I would think we would just want to keep the sorting logic so that monitors that are disabled get sorted to the bottom and just ignore the disabled in that case.
I would also probably not do this in this cleanup PR
There was a problem hiding this comment.
the issue is that we only read off the MonitorEnvironment status during sort right now, and that never gets marked as DISABLED. this is the only place we have that hook. some of this follow up work was going to simplify the sort logic but really it's more pending the "one row per monitor w/multiple environments" UI
There was a problem hiding this comment.
#48039
fix is here. will remove this
| @@ -99,6 +99,7 @@ def put(self, request: Request, organization, project, monitor) -> Response: | |||
| if "slug" in result: | |||
| params["slug"] = result["slug"] | |||
| if "status" in result: | |||
| # TODO(rjo100): resets status if monitor is failed, needs fixing with environments | |||
| if result["status"] == MonitorStatus.ACTIVE: | |||
There was a problem hiding this comment.
We're going to do a migration here to move all monitors that are in non ObjectStatus type status to ACTIVE or DISABLED right?
There was a problem hiding this comment.
ideally yes. we reference PENDING DELETION a lot too
There was a problem hiding this comment.
that's OK, pending deletion is part of ObjectStatus right
| @@ -218,43 +218,6 @@ def get_next_scheduled_checkin(self, last_checkin): | |||
| ) | |||
| return next_checkin + timedelta(minutes=int(self.config.get("checkin_margin") or 0)) | |||
|
|
|||
| def mark_failed(self, last_checkin=None, reason=MonitorFailure.UNKNOWN): | |||
| @@ -86,8 +86,6 @@ def serialize(self, obj, attrs, user): | |||
| "name": obj.name, | |||
| "slug": obj.slug, | |||
| "config": config, | |||
| "lastCheckIn": obj.last_checkin, | |||
| "nextCheckIn": obj.next_checkin, | |||
| def get_status_display(self) -> str: | ||
| for status_id, display in MonitorStatus.as_choices(): | ||
| if status_id in [ | ||
| ObjectStatus.ACTIVE, |
There was a problem hiding this comment.
I know this is duplicate but I'd rather keep it for readability sake
Removes references as best as possible to
next_checkin,last_checkin, andstatus(where appropriate) on theMonitorobject.Will require follow up work to remove hacks/TODOs as well as actually migrate the database and potentially backfill where appropriate.