Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions .agents/skills/generate-migration/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ sentry django makemigrations <app_name> --empty
3. Run `sentry django sqlmigrate <app_name> <migration_name>` to verify the SQL
4. Apply the migration locally with `sentry django migrate <app_name>` — Sentry's migration framework runs its safety checks on apply, so this catches unsafe ops (missing `is_post_deployment`, unsafe column changes, etc.) before CI does.

When editing a generated migration (e.g. swapping `DeleteModel` for `SafeDeleteModel`), **leave the auto-generated `is_post_deployment` comment block in place**. It documents a non-obvious flag with concrete guidance for future migration authors — useful context, not fluff. Only remove a comment if it's stale or contradicts the code.

## Guidelines

### Adding Columns
Expand All @@ -51,11 +53,20 @@ For large tables, set `is_post_deployment = True` on the migration as index crea
3. Replace `RemoveField` with `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)`
4. Deploy, then create second migration with `SafeRemoveField(..., deletion_action=DeletionAction.DELETE)`

### Deleting Tables
### Removing a Model (and eventually its table)

Two-phase process — the `historical_silo_assignments` entry must be added in phase 1.

**Phase 1 — Remove the model class (`MOVE_TO_PENDING`)**

1. Remove all code references
2. Replace `DeleteModel` with `SafeDeleteModel(..., deletion_action=DeletionAction.MOVE_TO_PENDING)`
3. Deploy, then create second migration with `SafeDeleteModel(..., deletion_action=DeletionAction.DELETE)`
3. Add the table to `historical_silo_assignments` in `src/sentry/db/router.py` (or `getsentry/db/router.py` for getsentry models). Pick the silo the model used — usually `SiloMode.CELL`.
4. Deploy

**Phase 2 — Drop the table (`DELETE`)**

After phase 1 has deployed, create a second migration with `SafeDeleteModel(..., deletion_action=DeletionAction.DELETE)`. Leave the historical entry in place — the table-drop migration relies on it to resolve the silo.

### Renaming Columns/Tables

Expand Down
15 changes: 12 additions & 3 deletions src/sentry/new_migrations/monkey/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,16 @@ def database_forwards(
from_state: SentryProjectState, # type: ignore[override]
to_state: SentryProjectState, # type: ignore[override]
) -> None:
# The historical_silo_assignments check must run for MOVE_TO_PENDING as
# well as DELETE. MOVE_TO_PENDING typically ships in the same PR that
# deletes the model class, after which the silo router can no longer
# resolve the table via the live app registry. Without a historical
# entry, allow_migrate returns False and the migration silently skips
# in prod (see #114929/dashboardlastvisited for a real incident).
if self.deletion_action == DeletionAction.MOVE_TO_PENDING:
return

model = from_state.get_pending_deletion_model(app_label, self.name)
model = from_state.apps.get_model(app_label, self.name)
else:
model = from_state.get_pending_deletion_model(app_label, self.name)
table = model._meta.db_table

# Check if we can determine the model's database to detect missing
Expand All @@ -58,6 +64,9 @@ def database_forwards(
f"with the appropriate SiloMode before the deletion migration can run. "
)

if self.deletion_action == DeletionAction.MOVE_TO_PENDING:
return

if self.allow_migrate_model(schema_editor.connection.alias, model):
schema_editor.delete_model(model, is_safe=True)

Expand Down
38 changes: 38 additions & 0 deletions tests/sentry/new_migrations/monkey/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,41 @@ def test_delete_model_without_historical_assignment_fails(self) -> None:
assert "Cannot determine database for deleted model" in str(exc_info.value)
assert "sentry_fake_deleted_table_not_in_router" in str(exc_info.value)
assert "historical_silo_assignments" in str(exc_info.value)

def test_move_to_pending_without_historical_assignment_fails(self) -> None:
"""
When marking a model for pending deletion that is not in
historical_silo_assignments and cannot be resolved via the live app
registry, SafeDeleteModel should raise a ValueError in test
environments. This catches the case where the model class is deleted
in the same PR as the MOVE_TO_PENDING migration without adding the
historical entry — which would silently skip the migration in prod.
"""
fake_meta = Mock()
fake_meta.db_table = "sentry_fake_pending_table_not_in_router"
fake_meta.app_label = "sentry"
fake_meta.model_name = "fakependingmodel"

FakePendingModel = Mock()
FakePendingModel._meta = fake_meta

from_state = Mock(spec=SentryProjectState)
from_state.apps.get_model.return_value = FakePendingModel
to_state = Mock(spec=SentryProjectState)

operation = SafeDeleteModel(
name="FakePendingModel", deletion_action=DeletionAction.MOVE_TO_PENDING
)

with connection.schema_editor() as schema_editor:
with pytest.raises(ValueError) as exc_info:
operation.database_forwards(
"sentry",
cast(SafePostgresDatabaseSchemaEditor, schema_editor),
from_state,
to_state,
)

assert "Cannot determine database for deleted model" in str(exc_info.value)
assert "sentry_fake_pending_table_not_in_router" in str(exc_info.value)
assert "historical_silo_assignments" in str(exc_info.value)
Loading