-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(eco): sentry app install deletion outbox #101896
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| from collections.abc import Collection, Mapping | ||
| from typing import TYPE_CHECKING, Any, ClassVar, overload | ||
|
|
||
| from django.db import models | ||
| from django.db import models, router, transaction | ||
| from django.utils import timezone | ||
| from jsonschema import ValidationError | ||
|
|
||
|
|
@@ -16,17 +16,17 @@ | |
| from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey | ||
| from sentry.db.models.manager.base_query_set import BaseQuerySet | ||
| from sentry.db.models.paranoia import ParanoidManager, ParanoidModel | ||
| from sentry.hybridcloud.models.outbox import ControlOutboxBase, outbox_context | ||
| from sentry.hybridcloud.models.outbox import ControlOutbox, ControlOutboxBase, outbox_context | ||
| from sentry.hybridcloud.outbox.base import ReplicatedControlModel | ||
| from sentry.hybridcloud.outbox.category import OutboxCategory | ||
| from sentry.hybridcloud.outbox.category import OutboxCategory, OutboxScope | ||
| from sentry.projects.services.project import RpcProject | ||
| from sentry.sentry_apps.services.app.model import RpcSentryAppComponent, RpcSentryAppInstallation | ||
| from sentry.sentry_apps.utils.errors import ( | ||
| SentryAppError, | ||
| SentryAppIntegratorError, | ||
| SentryAppSentryError, | ||
| ) | ||
| from sentry.types.region import find_regions_for_orgs | ||
| from sentry.types.region import find_all_region_names, find_regions_for_orgs | ||
|
|
||
| if TYPE_CHECKING: | ||
| from sentry.models.project import Project | ||
|
|
@@ -124,6 +124,12 @@ def save(self, *args, **kwargs): | |
| self.date_updated = timezone.now() | ||
| return super().save(*args, **kwargs) | ||
|
|
||
| def delete(self, *args, **kwargs): | ||
| with outbox_context(transaction.atomic(using=router.db_for_write(SentryAppInstallation))): | ||
| for outbox in self.outboxes_for_delete(): | ||
| outbox.save() | ||
| return super().delete(*args, **kwargs) | ||
|
|
||
| @property | ||
| def api_application_id(self) -> int | None: | ||
| from sentry.sentry_apps.models.sentry_app import SentryApp | ||
|
|
@@ -141,6 +147,19 @@ def outboxes_for_update(self, shard_identifier: int | None = None) -> list[Contr | |
| # these isn't so important in that case. | ||
| return super().outboxes_for_update(shard_identifier=self.api_application_id or 0) | ||
|
|
||
| def outboxes_for_delete(self) -> list[ControlOutbox]: | ||
| return [ | ||
| ControlOutbox( | ||
| shard_scope=OutboxScope.APP_SCOPE, | ||
| shard_identifier=self.id, | ||
| object_identifier=self.id, | ||
| category=OutboxCategory.SENTRY_APP_INSTALLATION_DELETE, | ||
| region_name=region_name, | ||
| payload={"uuid": self.uuid}, | ||
| ) | ||
| for region_name in find_all_region_names() | ||
|
Comment on lines
151
to
160
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes look good, just needs testing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh does this still need a new
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes I found out while testing lmao |
||
| ] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Inconsistent Region Targeting in Delete MethodThe
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The organization might already be deleted by the time we get here in the sentry app / install deletion task. See #101895 |
||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Outbox Processing InconsistencyThe |
||
| def prepare_ui_component( | ||
| self, | ||
| component: SentryAppComponent, | ||
|
|
||
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.
Can we use the same shard_identifier as update? Having the same sharding keys ensures that all the outboxes for a specific record are processed in order.