Skip to content
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

feat(migrations): Prevent new migrations from using RunSQL #81003

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

wedamija
Copy link
Member

@wedamija wedamija commented Nov 19, 2024

This adds in allow_run_sql to our CheckedMigration baseclass. We want to prevent the use of RunSQL in most cases going forward, since our migration safety framework doesn't know how to parse sql to ensure safety.

Our main uses for this in the past were:

  • Adding new columns with a default. We now can do this within the framework by using db_default instead of default.
  • Deleting columns/tables. Since we have to remove from state, deploy, then delete we have to use raw sql to remove them from Postgres. I'll follow this up with a pr to provide ways to handle these deletions within our framework.

@wedamija wedamija requested review from a team as code owners November 19, 2024 20:10
@wedamija
Copy link
Member Author

Note that some tests will fail until we have a way to handle deleting tables/cols

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 19, 2024
Copy link

codecov bot commented Nov 19, 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   #81003      +/-   ##
==========================================
+ Coverage   80.32%   80.34%   +0.01%     
==========================================
  Files        7214     7215       +1     
  Lines      319316   319405      +89     
  Branches    20775    20775              
==========================================
+ Hits       256504   256613     +109     
+ Misses      62418    62398      -20     
  Partials      394      394              

This adds in `allow_run_sql` to our `CheckedMigration` baseclass. We want to prevent the use of `RunSQL` in most cases going forward, since our migration safety framework doesn't know how to parse sql to ensure safety.

Our main uses for this in the past were:
 - Adding new columns with a default. We now can do this within the framework by using `db_default` instead of `default.
 - Deleting columns/tables. Since we have to remove from state, deploy, then delete we have to use raw sql to remove them from Postgres. I'll follow this up with a pr to provide ways to handle these deletions within our framework.
@wedamija wedamija force-pushed the danf/migrations-prevent-run-sql branch from fe6f41d to 5539359 Compare November 22, 2024 19:23
@wedamija wedamija enabled auto-merge (squash) November 22, 2024 19:24
@wedamija wedamija merged commit 64c5f94 into master Nov 22, 2024
50 checks passed
@wedamija wedamija deleted the danf/migrations-prevent-run-sql branch November 22, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants