ref(rules): Migrate 'bad' rules to be disabled #55941

ceorourke · 2023-09-08T20:49:55Z

Migrate alert rules that have no action or are exact duplicates of another rule in the same project to be disabled. We now prevent these from being created, this will just clean up old ones.

Closes #55502

src/sentry/migrations/0546_migrate_no_action_dupe_issue_alerts.py

github-actions · 2023-09-08T20:52:36Z

This PR has a migration; here is the generated SQL for src/sentry/migrations/0546_migrate_no_action_dupe_issue_alerts.py ()

--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--

codecov · 2023-09-08T21:22:33Z

Codecov Report

Merging #55941 (a3c8931) into master (36121c1) will decrease coverage by 0.01%.
Report is 6 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #55941      +/-   ##
==========================================
- Coverage   79.99%   79.98%   -0.01%     
==========================================
  Files        5063     5063              
  Lines      217841   217848       +7     
  Branches    36879    36881       +2     
==========================================
- Hits       174254   174250       -4     
- Misses      38243    38249       +6     
- Partials     5344     5349       +5

Files Changed	Coverage
src/sentry/testutils/factories.py	`100.00%`

src/sentry/migrations/0546_migrate_no_action_dupe_issue_alerts.py

github-actions · 2023-09-11T18:26:40Z

This PR has a migration; here is the generated SQL for src/sentry/migrations/0548_migrate_no_action_dupe_issue_alerts.py ()

--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--

github-actions · 2023-09-11T19:14:56Z

This PR has a migration; here is the generated SQL for src/sentry/migrations/0549_migrate_no_action_dupe_issue_alerts.py ()

--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--

JoshFerge

is it possible to write a test for this? I know we previously added some tooling around data migration tests

ceorourke · 2023-09-11T19:23:10Z

is it possible to write a test for this? I know we previously added some tooling around data migration tests

I can - could you point me to where that is? I'm not finding much for test_migration, /tests/migrations etc.

JoshFerge · 2023-09-11T19:36:51Z

is it possible to write a test for this? I know we previously added some tooling around data migration tests

I can - could you point me to where that is? I'm not finding much for test_migration, /tests/migrations etc.

Let me know if there's enough context here: https://develop.sentry.dev/database-migrations/#testing

ceorourke · 2023-09-11T20:04:17Z

is it possible to write a test for this? I know we previously added some tooling around data migration tests

I can - could you point me to where that is? I'm not finding much for test_migration, /tests/migrations etc.

Let me know if there's enough context here: https://develop.sentry.dev/database-migrations/#testing

I added a test based on that but running it locally (even with the env var set) always says: Migration is no longer runnable. Retain until migration is removed. 😞 This happens on almost every other test in the directory... might be because 0547 has kinda borked my local env.

Update: Fixed 0547 locally and I get the same message.

JoshFerge · 2023-09-11T20:37:58Z

is it possible to write a test for this? I know we previously added some tooling around data migration tests

I can - could you point me to where that is? I'm not finding much for test_migration, /tests/migrations etc.

Let me know if there's enough context here: https://develop.sentry.dev/database-migrations/#testing

I added a test based on that but running it locally (even with the env var set) always says: Migration is no longer runnable. Retain until migration is removed. 😞 This happens on almost every other test in the directory... might be because 0547 has kinda borked my local env.

Update: Fixed 0547 locally and I get the same message.

ah annoying! i'll try running it on my local in a sec and see what happens. thanks for adding that!

ceorourke · 2023-09-11T20:47:46Z

is it possible to write a test for this? I know we previously added some tooling around data migration tests

I can - could you point me to where that is? I'm not finding much for test_migration, /tests/migrations etc.

Let me know if there's enough context here: https://develop.sentry.dev/database-migrations/#testing

I added a test based on that but running it locally (even with the env var set) always says: Migration is no longer runnable. Retain until migration is removed. 😞 This happens on almost every other test in the directory... might be because 0547 has kinda borked my local env.
Update: Fixed 0547 locally and I get the same message.

ah annoying! i'll try running it on my local in a sec and see what happens. thanks for adding that!

Oh duh - they're all marked as skippable: @pytest.mark.skip("Migration is no longer runnable. Retain until migration is removed.") why is that though?

tests/sentry/migrations/test_0549_migrate_no_action_dupe_issue_alerts.py

JoshFerge · 2023-09-11T21:53:51Z

is it possible to write a test for this? I know we previously added some tooling around data migration tests

I can - could you point me to where that is? I'm not finding much for test_migration, /tests/migrations etc.

Let me know if there's enough context here: https://develop.sentry.dev/database-migrations/#testing

I added a test based on that but running it locally (even with the env var set) always says: Migration is no longer runnable. Retain until migration is removed. 😞 This happens on almost every other test in the directory... might be because 0547 has kinda borked my local env.
Update: Fixed 0547 locally and I get the same message.

ah annoying! i'll try running it on my local in a sec and see what happens. thanks for adding that!

Oh duh - they're all marked as skippable: @pytest.mark.skip("Migration is no longer runnable. Retain until migration is removed.") why is that though?

@markstory do you know on this one? do we generally make them skip after the PR is merged?

src/sentry/migrations/0549_migrate_no_action_dupe_issue_alerts.py

github-actions · 2023-09-12T18:19:24Z

This PR has a migration; here is the generated SQL for src/sentry/migrations/0550_migrate_no_action_dupe_issue_alerts.py ()

--
-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:
-- Raw Python operation
--

#55941 had a mistake where rules that were the exact same in the same project that had different environments were being disabled due to a discrepancy in how we need to compare rule data in the app (a data dict including the environment data, if set) versus how we need to compare rule data in the database (if the environment is set, it's a column in the table, not in `data`). This migration will find disabled rules with an environment id set and compare them against other rules in the project. If they're actually duplicates (including the environment) we'll leave it alone, but if they're duplicates _except_ for the environment, we'll re-enable it.

ceorourke requested a review from a team as a code owner September 8, 2023 20:49

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 8, 2023

ceorourke commented Sep 8, 2023

View reviewed changes

src/sentry/migrations/0546_migrate_no_action_dupe_issue_alerts.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview September 8, 2023 20:51 View deployment

markstory reviewed Sep 11, 2023

View reviewed changes

src/sentry/migrations/0546_migrate_no_action_dupe_issue_alerts.py Outdated Show resolved Hide resolved

src/sentry/migrations/0546_migrate_no_action_dupe_issue_alerts.py Outdated Show resolved Hide resolved

ceorourke force-pushed the ceorourke/migrate-bad-alerts branch from d6819b4 to 4a7c3f4 Compare September 11, 2023 18:24

vercel bot deployed to Preview September 11, 2023 18:27 View deployment

ceorourke requested a review from markstory September 11, 2023 19:10

ceorourke force-pushed the ceorourke/migrate-bad-alerts branch from 4a7c3f4 to f50ed55 Compare September 11, 2023 19:12

vercel bot deployed to Preview September 11, 2023 19:14 View deployment

JoshFerge reviewed Sep 11, 2023

View reviewed changes

vercel bot deployed to Preview September 11, 2023 20:43 View deployment

ceorourke commented Sep 11, 2023

View reviewed changes

tests/sentry/migrations/test_0549_migrate_no_action_dupe_issue_alerts.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview September 11, 2023 21:27 View deployment

vercel bot deployed to Preview September 11, 2023 22:28 View deployment

JoshFerge reviewed Sep 11, 2023

View reviewed changes

src/sentry/migrations/0549_migrate_no_action_dupe_issue_alerts.py Outdated Show resolved Hide resolved

vercel bot deployed to Preview September 11, 2023 22:51 View deployment

vercel bot deployed to Preview September 12, 2023 00:01 View deployment

ceorourke added 2 commits September 12, 2023 11:14

ref(rules): Migrate 'bad' rules to be disabled

5ea04cb

rebase

8b5e145

ceorourke added 8 commits September 12, 2023 11:14

rebase again

04636d7

add test

15d7293

update test

0f7d2da

update test

c41fed0

copy objectstatus enum into migration

641fc9b

oops logic

4ee53af

slightly wrong order of rebase steps

78c3864

rebase again

a3c8931

ceorourke force-pushed the ceorourke/migrate-bad-alerts branch from 9ccc76f to a3c8931 Compare September 12, 2023 18:16

markstory approved these changes Sep 12, 2023

View reviewed changes

vercel bot deployed to Preview September 12, 2023 18:20 View deployment

ceorourke merged commit a452b23 into master Sep 12, 2023
52 checks passed

ceorourke deleted the ceorourke/migrate-bad-alerts branch September 12, 2023 21:19

ceorourke mentioned this pull request Sep 22, 2023

ref(alerts): Fix status of dupe but diff env alerts #56763

Merged

github-actions bot locked and limited conversation to collaborators Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(rules): Migrate 'bad' rules to be disabled #55941

ref(rules): Migrate 'bad' rules to be disabled #55941

ceorourke commented Sep 8, 2023 •

edited

Loading

github-actions bot commented Sep 8, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

JoshFerge left a comment

ceorourke commented Sep 11, 2023

JoshFerge commented Sep 11, 2023

ceorourke commented Sep 11, 2023 •

edited

Loading

JoshFerge commented Sep 11, 2023

ceorourke commented Sep 11, 2023

JoshFerge commented Sep 11, 2023

github-actions bot commented Sep 12, 2023

ref(rules): Migrate 'bad' rules to be disabled #55941

ref(rules): Migrate 'bad' rules to be disabled #55941

Conversation

ceorourke commented Sep 8, 2023 • edited Loading

github-actions bot commented Sep 8, 2023

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

JoshFerge left a comment

Choose a reason for hiding this comment

ceorourke commented Sep 11, 2023

JoshFerge commented Sep 11, 2023

ceorourke commented Sep 11, 2023 • edited Loading

JoshFerge commented Sep 11, 2023

ceorourke commented Sep 11, 2023

JoshFerge commented Sep 11, 2023

github-actions bot commented Sep 12, 2023

ceorourke commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

ceorourke commented Sep 11, 2023 •

edited

Loading