Skip to content

fix(backup): Fix mis-migrated AlertRule imports#62526

Merged
azaslavsky merged 2 commits intomasterfrom
az/fix-actor-imports
Jan 3, 2024
Merged

fix(backup): Fix mis-migrated AlertRule imports#62526
azaslavsky merged 2 commits intomasterfrom
az/fix-actor-imports

Conversation

@azaslavsky
Copy link
Copy Markdown
Contributor

In theory, as part of the ongoing "actor refactor", all AlertRule instances with an owner_id pointing to a non-null Actor should have that Actor instance's populated user_id or team_id field hoisted into the AlertRule model (see #58667 for more). In practice, we've seen some failing relocations of real self-hosted data, implying that there have been some misses in the migration, causing newly imported models to fail validation.

In the long term, we'd like to implement a migration that fixes all of the missing edge cases. Until that happens, this import-time fix should allow us to work around the issue. The basic logic is to manually do the hoisting at import-time if possible. If not possible (either due to a missing owner_id, or because the Actor being pointed to both team and user set to null), we ensure that the owner_id is nulled out.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 3, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bf24a1a) 81.25% compared to head (20dab6c) 81.25%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #62526   +/-   ##
=======================================
  Coverage   81.25%   81.25%           
=======================================
  Files        5197     5197           
  Lines      230098   230110   +12     
  Branches    39745    39748    +3     
=======================================
+ Hits       186960   186975   +15     
+ Misses      37399    37398    -1     
+ Partials     5739     5737    -2     
Files Coverage Δ
src/sentry/incidents/models.py 96.62% <100.00%> (+0.08%) ⬆️
static/app/components/events/eventReplay/index.tsx 71.42% <ø> (ø)

... and 5 files with indirect coverage changes

@azaslavsky azaslavsky marked this pull request as ready for review January 3, 2024 17:28
@azaslavsky azaslavsky requested a review from a team as a code owner January 3, 2024 17:28
@azaslavsky azaslavsky requested review from a team and corps January 3, 2024 17:28
@azaslavsky azaslavsky changed the title feat(backup): Fix mis-migrated AlertRule imports fix(backup): Fix mis-migrated AlertRule imports Jan 3, 2024
@azaslavsky azaslavsky enabled auto-merge (squash) January 3, 2024 20:26
@azaslavsky azaslavsky merged commit dfa528f into master Jan 3, 2024
@azaslavsky azaslavsky deleted the az/fix-actor-imports branch January 3, 2024 20:58
armenzg pushed a commit that referenced this pull request Jan 8, 2024
In theory, as part of the ongoing "actor refactor", all `AlertRule`
instances with an `owner_id` pointing to a non-null `Actor` should have
that `Actor` instance's populated `user_id` or `team_id` field hoisted
into the `AlertRule` model (see
#58667 for more). In practice,
we've seen some failing relocations of real self-hosted data, implying
that there have been some misses in the migration, causing newly
imported models to fail validation.

In the long term, we'd like to implement a migration that fixes all of
the missing edge cases. Until that happens, this import-time fix should
allow us to work around the issue. The basic logic is to manually do the
hoisting at import-time if possible. If not possible (either due to a
missing `owner_id`, or because the `Actor` being pointed to both `team`
and `user` set to null), we ensure that the `owner_id` is nulled out.
azaslavsky added a commit that referenced this pull request Jan 10, 2024
Recently, we've added a couple of import handlers where we selectively
null out fields based on certain conditions (see: PRs #62665 and
#62526). This commit adds comparator support for this condition,
allowing us to define comparators that say "make sure these fields are
either equal or have been removed on the right side".
@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants