Skip to content

Conversation

@ceorourke
Copy link
Member

Another follow up to #81522 to remove the unused models. A final PR will follow to drop the tables.

@ceorourke ceorourke requested a review from a team as a code owner December 3, 2024 19:01
@ceorourke ceorourke requested a review from a team December 3, 2024 19:01
@ceorourke ceorourke requested a review from a team as a code owner December 3, 2024 19:01
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 3, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2024

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

--
-- Alter field incident on incidentseen
--
SET CONSTRAINTS "sentry_incidentseen_incident_id_008cf29b_fk_sentry_incident_id" IMMEDIATE; ALTER TABLE "sentry_incidentseen" DROP CONSTRAINT "sentry_incidentseen_incident_id_008cf29b_fk_sentry_incident_id";
--
-- Alter field user_id on incidentseen
--
ALTER TABLE "sentry_incidentseen" ALTER COLUMN "user_id" DROP NOT NULL;
--
-- Alter field incident on incidentsubscription
--
SET CONSTRAINTS "sentry_incidentsubsc_incident_id_6d439453_fk_sentry_in" IMMEDIATE; ALTER TABLE "sentry_incidentsubscription" DROP CONSTRAINT "sentry_incidentsubsc_incident_id_6d439453_fk_sentry_in";
--
-- Alter field user_id on incidentsubscription
--
ALTER TABLE "sentry_incidentsubscription" ALTER COLUMN "user_id" DROP NOT NULL;
--
-- Moved model IncidentSeen to pending deletion state
--
-- (no-op)
--
-- Moved model IncidentSubscription to pending deletion state
--
-- (no-op)

field=sentry.db.models.fields.hybrid_cloud_foreign_key.HybridCloudForeignKey(
"sentry.User", db_index=True, null=True, on_delete="CASCADE"
),
),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I generated the 2nd part of removing the models from code it also generated this:

migrations.AlterUniqueTogether(
    name="incidentsubscription",
    unique_together=None,
),
SafeRemoveField(
    model_name="incidentsubscription",
    name="incident",
    deletion_action=DeletionAction.MOVE_TO_PENDING,
),

but it did the same in #81264 and I had removed it without issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're going to be dropping the table shortly, the AlterUniqueTogether operation isn't that important as postgres will drop the constraint when the table is dropped.

@codecov
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #81605      +/-   ##
==========================================
+ Coverage   80.45%   80.50%   +0.05%     
==========================================
  Files        7241     7241              
  Lines      321452   322387     +935     
  Branches    20807    20807              
==========================================
+ Hits       258621   259537     +916     
- Misses      62432    62451      +19     
  Partials      399      399              

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Dec 3, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2024

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, constraints are being removed and columns made nullable alongside the state update for table removal 👍

field=sentry.db.models.fields.hybrid_cloud_foreign_key.HybridCloudForeignKey(
"sentry.User", db_index=True, null=True, on_delete="CASCADE"
),
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're going to be dropping the table shortly, the AlterUniqueTogether operation isn't that important as postgres will drop the constraint when the table is dropped.

@ceorourke ceorourke merged commit c06312d into master Dec 3, 2024
50 checks passed
@ceorourke ceorourke deleted the ceorourke/del-incidentseen-incidentsubscription branch December 3, 2024 21:41
ceorourke added a commit that referenced this pull request Dec 4, 2024
The final follow up to #81605 to
drop the `IncidentSeen` and `IncidentSubscription` tables.
@github-actions github-actions bot locked and limited conversation to collaborators Dec 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 Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants