Skip to content

feat(migrations): Add in SafeRemoveField migration operation #81098

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

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

wedamija
Copy link
Member

@wedamija wedamija commented Nov 21, 2024

This builds on #81063

This adds in SafeRemoveField, which works the same way as SafeRemoveField, except on database columns.

It performs checks that the column doesn't have a db constraint set, and also that it is either nullable or has a db_default set.

Similarly to SafeRemoveField we still need to be careful to make sure that the pending deletion merges and deploys first, and then the real deletion.

@wedamija wedamija requested review from a team November 21, 2024 00:48
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 21, 2024
@wedamija
Copy link
Member Author

Note that this pr looks huge, but it's mostly just boilerplate from tests. I recommend marking all the tests viewed to focus on the core changes

@wedamija wedamija force-pushed the danf/migrations-safe-deletes branch from a3cfac3 to e187edf Compare November 21, 2024 22:49
@wedamija wedamija force-pushed the danf/migrations-safe-col-deletes branch from 19ca00a to 6d9aeb4 Compare November 21, 2024 22:52
Base automatically changed from danf/migrations-safe-deletes to master November 22, 2024 17:04
This builds on #81063

This adds in `SafeDeleteColumn`, which works the same way as `SafeDeleteModel`, except on database columns.

It performs checks that the column doesn't have a db constraint set, and also that it is either nullable or has a db_default set.

Similarly to `SafeDeleteModel` we still need to be careful to make sure that the pending deletion merges and deploys first, and then the real deletion.
@wedamija wedamija force-pushed the danf/migrations-safe-col-deletes branch from f71a44d to 2753829 Compare November 22, 2024 17:07
@wedamija wedamija requested a review from markstory November 22, 2024 17:07
wedamija added a commit to getsentry/sentry-docs that referenced this pull request Nov 22, 2024
@wedamija wedamija enabled auto-merge (squash) November 22, 2024 18:34
@wedamija wedamija changed the title feat(migrations): Add in SafeDeleteColumn migration operation feat(migrations): Add in SafeRemoveField migration operation Nov 22, 2024
@wedamija wedamija merged commit bff86bb into master Nov 22, 2024
51 of 52 checks passed
@wedamija wedamija deleted the danf/migrations-safe-col-deletes branch November 22, 2024 18:53
wedamija added a commit to getsentry/sentry-docs that referenced this pull request Nov 22, 2024
harshithadurai pushed a commit that referenced this pull request Nov 25, 2024
This builds on #81063

This adds in `SafeDeleteColumn`, which works the same way as
`SafeDeleteModel`, except on database columns.

It performs checks that the column doesn't have a db constraint set, and
also that it is either nullable or has a db_default set.

Similarly to `SafeDeleteModel` we still need to be careful to make sure
that the pending deletion merges and deploys first, and then the real
deletion.
ceorourke added a commit that referenced this pull request Nov 25, 2024
In preparation for removing the `AlertRuleExcludedProjects` model (see
#81020) we need to first remove
the many to many `excluded_projects` column on the `AlertRule` model. We
can take the opportunity to remove the now unused `include_all_projects`
column. This PR uses the brand new `SafeRemoveField` option added in
#81098
evanh pushed a commit that referenced this pull request Nov 25, 2024
This builds on #81063

This adds in `SafeDeleteColumn`, which works the same way as
`SafeDeleteModel`, except on database columns.

It performs checks that the column doesn't have a db constraint set, and
also that it is either nullable or has a db_default set.

Similarly to `SafeDeleteModel` we still need to be careful to make sure
that the pending deletion merges and deploys first, and then the real
deletion.
evanh pushed a commit that referenced this pull request Nov 25, 2024
In preparation for removing the `AlertRuleExcludedProjects` model (see
#81020) we need to first remove
the many to many `excluded_projects` column on the `AlertRule` model. We
can take the opportunity to remove the now unused `include_all_projects`
column. This PR uses the brand new `SafeRemoveField` option added in
#81098
andrewshie-sentry pushed a commit that referenced this pull request Dec 2, 2024
This builds on #81063

This adds in `SafeDeleteColumn`, which works the same way as
`SafeDeleteModel`, except on database columns.

It performs checks that the column doesn't have a db constraint set, and
also that it is either nullable or has a db_default set.

Similarly to `SafeDeleteModel` we still need to be careful to make sure
that the pending deletion merges and deploys first, and then the real
deletion.
andrewshie-sentry pushed a commit that referenced this pull request Dec 2, 2024
In preparation for removing the `AlertRuleExcludedProjects` model (see
#81020) we need to first remove
the many to many `excluded_projects` column on the `AlertRule` model. We
can take the opportunity to remove the now unused `include_all_projects`
column. This PR uses the brand new `SafeRemoveField` option added in
#81098
Copy link

sentry-io bot commented Dec 3, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ CrossTransactionAssertionError: Transaction opened for db {'secondary'}, but command running against db default pytest.runtest.protocol tests/sentry/db/postgre... View Issue

Did you find this useful? React with a 👍 or 👎

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants