Update docs on how to delete tables and columns #11897
Conversation
This uses the new operations from getsentry/sentry#81063 getsentry/sentry#81098
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
| - If the column is a foreign key, remove the database level foreign key constraint it by setting `db_constraint=False`. | ||
| - Remove the column and in the generated migration use `SafeRemoveField(..., deletion_action=DeletionAction.MOVE_TO_PENDING)` to replace `RemoveField(...)`. This only marks the state for the column as removed. | ||
| - Combine these migrations together to save making multiple deploys | ||
| - Deploy. It's important that all previous prs are in production before we remove the actual column from the table. |
There was a problem hiding this comment.
Should we call this out even more explicitly, maybe a <Note> type thing?
There was a problem hiding this comment.
I'm not sure if it's necessary, people should just be following these steps in order, and we've had the docs like this for a while
develop-docs/api-server/application-domains/database-migrations/index.mdx
Outdated
Show resolved
Hide resolved
develop-docs/api-server/application-domains/database-migrations/index.mdx
Outdated
Show resolved
Hide resolved
develop-docs/api-server/application-domains/database-migrations/index.mdx
Outdated
Show resolved
Hide resolved
develop-docs/api-server/application-domains/database-migrations/index.mdx
Outdated
Show resolved
Hide resolved
develop-docs/api-server/application-domains/database-migrations/index.mdx
Outdated
Show resolved
Hide resolved
develop-docs/api-server/application-domains/database-migrations/index.mdx
Outdated
Show resolved
Hide resolved
develop-docs/api-server/application-domains/database-migrations/index.mdx
Outdated
Show resolved
Hide resolved
…s/index.mdx Co-authored-by: Mark Story <[email protected]>
…s/index.mdx Co-authored-by: Mark Story <[email protected]>
…s/index.mdx Co-authored-by: Mark Story <[email protected]>
…s/index.mdx Co-authored-by: Mark Story <[email protected]>
…s/index.mdx Co-authored-by: Mark Story <[email protected]>
…s/index.mdx Co-authored-by: Mark Story <[email protected]>
…s/index.mdx Co-authored-by: Mark Story <[email protected]>
| - Remove the column from the model, but in the migration make sure we only mark the state as removed. | ||
| - Deploy. | ||
| - Finally, create a migration that deletes the column. | ||
| - (Optional, but ideal) Make a PR to remove all uses of the column in the codebase in a separate PR. This mostly helps with code cleanliness. This should be merged ahead of the migration prs, but we don't need to worry about whether it is deployed first. |
There was a problem hiding this comment.
I'm slightly confused about the optionality of this, is it just optional that it's a separate PR. It kind of makes it sound like we don't need to remove all the uses of the column, we definitely do.
I actually wouldn't mind if this just was the prescribed flow and remove the note about optionality.
| - (Optional, but ideal) Make a PR to remove all uses of the column in the codebase in a separate PR. This mostly helps with code cleanliness. This should be merged ahead of the migration prs, but we don't need to worry about whether it is deployed first. | |
| - Make a PR to remove all uses of the column in the codebase in a separate PR. This mostly helps with code cleanliness. This should be merged ahead of the migration prs, but we don't need to worry about whether it is deployed first. |
| - Finally, create a migration that deletes the column. | ||
| - (Optional, but ideal) Make a PR to remove all uses of the column in the codebase in a separate PR. This mostly helps with code cleanliness. This should be merged ahead of the migration prs, but we don't need to worry about whether it is deployed first. | ||
| - Make another PR that: | ||
| - Checks if the column is either not nullable, or doesn't have a db_default set. If either of these is true, then make it nullable via `null=True`. |
There was a problem hiding this comment.
| - Checks if the column is either not nullable, or doesn't have a db_default set. If either of these is true, then make it nullable via `null=True`. | |
| - Checks if the column is either not nullable, or doesn't have a `db_default` set. If either of these is true, then make it nullable via `null=True`. |
|
|
||
| ```python | ||
| operations = [ | ||
| SafeRemoveField(model_name="testmodel", name="project", deletion_action=DeletionAction.MOVE_TO_PENDING), |
There was a problem hiding this comment.
Do you have to import SafeRemoveField? Should we monkey-patch it onto the migrations module?
There was a problem hiding this comment.
Hmm, monkeypatching it in feels a bit weird... Usually I just quick import things using my IDE, so I'm not sure it's good to do even more monkeying here?
There was a problem hiding this comment.
Yeah sounds good. Let’s at least add the import to the code example though
| ] | ||
| ``` | ||
|
|
||
| Using `SafeRemoveField` allows us to remove all the state related to the column, but defer deleting it until a later migration. So now, we can combine the migration to remove the constraints and delete the column state together like so |
There was a problem hiding this comment.
It's a little confusing here that the next codeblock we're saying "That second migration you just made, throw that one away, we can combine them into the first"
We do need to describe both, since in the case where there's no alter needed for removing null or FKs, we just want the safe remove, but I feel like we should tweak this a bit to make it more clear that that's whats happening here
There was a problem hiding this comment.
Maybe it'd help to just post the two migrations necessary first, then write the steps to get to that state after?
There was a problem hiding this comment.
I think that would make sense
This uses the new operations from
getsentry/sentry#81063
getsentry/sentry#81098