-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(preprod): drop existing extras column on PreprodArtifact #93388
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #93388 +/- ##
===========================================
+ Coverage 41.27% 88.00% +46.72%
===========================================
Files 10259 10289 +30
Lines 592078 593525 +1447
Branches 23028 23028
===========================================
+ Hits 244401 522311 +277910
+ Misses 347245 70782 -276463
Partials 432 432 |
This PR has a migration; here is the generated SQL for for --
-- Moved preprodartifact.extras field to pending deletion state
--
-- (no-op) |
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.
Moved this other migration test to our project.
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.
Looks correct to me 👍. And since the table is empty I don't foresee there being any trouble whatsoever. Pinging @wedamija in case he has thoughts. But LGTM.
To clarify we have ~6 records in the table, but |
from sentry.testutils.cases import TestMigrations | ||
|
||
|
||
class DropSentryJsonfieldTests(TestMigrations): |
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.
No need to write migration tests for table changes. These are just for backfills and so on that have logic, better to remove this since these tend to be slow and a break over time
Second step of the plan in #93388 Won't merge until the first PR is fully deployed.
Second step of the plan in #93388 Won't merge until the first PR is fully deployed.
<!-- Describe your PR here. --> In #92056 (comment) we were requested to switch to using Django's JSONField. I don't remember anything in particular for why we chose Sentry's version, so seems fine to me! Since this is renaming a column type (text -> jsonb) this requires a few migrations ([based on this)](https://develop.sentry.dev/backend/application-domains/database-migrations/#deleting-columns): 1) (this PR) drop the existing column in safe mode. this is fine because we don't have any prod data for this table yet. 2) actually drop the column 3) add back the column with the new type
Second step of the plan in #93388 Won't merge until the first PR is fully deployed.
In #92056 (comment) we were requested to switch to using Django's JSONField. I don't remember anything in particular for why we chose Sentry's version, so seems fine to me!
Since this is renaming a column type (text -> jsonb) this requires a few migrations (based on this):