-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(deletions): Move tasks under sentry.deletions #104009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This is for consistencies sake.
| }, | ||
| "delete-pending-groups": { | ||
| "task": "deletions:sentry.tasks.delete_pending_groups", | ||
| "task": "deletions:sentry.deletions.tasks.delete_pending_groups", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markstory will something break if I rename this task?
| "sentry.tasks.beacon", | ||
| "sentry.tasks.codeowners.*", | ||
| "sentry.tasks.commit_context", | ||
| "sentry.tasks.delete_seer_grouping_records", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sentry.deletions.* is strongly typed, thus, no need to list this module explicitely.
❌ 12 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Task name not updated in decorator
The task name in the @instrumented_task decorator for call_seer_delete_project_grouping_records wasn't updated from sentry.tasks.call_seer_delete_project_grouping_records to sentry.deletions.tasks.call_seer_delete_project_grouping_records. This inconsistency means the task is registered under the old namespace instead of the new sentry.deletions namespace, defeating the purpose of this refactoring and potentially causing task routing issues if the old task name is referenced elsewhere in the codebase.
src/sentry/deletions/tasks/delete_seer_grouping_records.py#L88-L89
sentry/src/sentry/deletions/tasks/delete_seer_grouping_records.py
Lines 88 to 89 in 40804a5
| @instrumented_task( | |
| name="sentry.tasks.call_seer_delete_project_grouping_records", |
Bug: Outdated patch decorator references old module path
Several @patch decorators in test files still reference the old module paths like sentry.tasks.delete_pending_groups.metrics.incr and sentry.tasks.delete_seer_grouping_records.delete_seer_grouping_records_by_hash.apply_async instead of the new sentry.deletions.tasks.* paths. These patches won't work correctly because the modules have been moved, causing the tests to either fail or not properly mock the intended functions.
tests/sentry/deletions/tasks/test_delete_pending_groups.py#L116-L117
sentry/tests/sentry/deletions/tasks/test_delete_pending_groups.py
Lines 116 to 117 in 40804a5
| @patch("sentry.api.helpers.group_index.delete.delete_groups_for_project.apply_async") | |
| @patch("sentry.tasks.delete_pending_groups.metrics.incr") |
tests/sentry/deletions/tasks/test_delete_seer_grouping_records.py#L30-L31
| @patch( | |
| "sentry.tasks.delete_seer_grouping_records.delete_seer_grouping_records_by_hash.apply_async" |
tests/sentry/deletions/tasks/test_delete_seer_grouping_records.py#L51-L52
| patch( | |
| "sentry.tasks.delete_seer_grouping_records.delete_seer_grouping_records_by_hash.apply_async" |
I want tasks dealing with deletion to start with
sentry.deletionsto have consistency with the rest and find them more easily.