-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
fix: remove unneeded complexity in migration #19022
fix: remove unneeded complexity in migration #19022
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19022 +/- ##
=======================================
Coverage 66.56% 66.56%
=======================================
Files 1641 1641
Lines 63495 63495
Branches 6425 6425
=======================================
Hits 42265 42265
Misses 19550 19550
Partials 1680 1680
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
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.
wow.. that's a lot less code. :)
Yeah! The original implementation was copied from the current |
🏷️ preset:2022.9 |
(cherry picked from commit 50bb86d)
(cherry picked from commit 50bb86d)
SUMMARY
The migration from SIP-68 was calling
create_engine
just to get the dialect associated with a given database. This required reimplementing many methods in the customDatabase
class defined in the migration, since it requires doing user impersonation, mutating the connection string, and much more. This was failing for BigQuery because it was not finding the credentials:Turns out there's a simpler way of getting the dialect associated with a database that doesn't require
create_engine
. I implemented it in this PR.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
Migration should run fine. I ran a downgrade and upgrade and it works fine, but I only testes with Postgres.
ADDITIONAL INFORMATION