-
Notifications
You must be signed in to change notification settings - Fork 55
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
Remove gorm from the migrator #3006
Conversation
1cef953
to
fcffa8a
Compare
Add a shim that allows it to continue to test using the gorm DB. When we change the data package to create a postgres connection directly we can change the migrator as well. Also remove transactions from migrator Transactions can be managed by the caller. This will make it easier to switch away from gorm.
fcffa8a
to
0c96d3f
Compare
|
||
opts := migrator.Options{ | ||
InitSchema: initializeSchema, | ||
LoadKey: loadDBKey, | ||
LoadKey: func(tx migrator.DB) error { | ||
if loadDBKey == nil { |
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.
This is nil in the case of tests?
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.
Ya, exactly
stmt := `ALTER TABLE settings DROP COLUMN IF EXISTS signup_enabled` | ||
if tx.Dialector.Name() == "sqlite" { | ||
if tx.DriverName() == "sqlite" { |
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.
Not related to this change but you mentioned it in the description, I think we should drop the sqlite checks soon. The table alterations already don't work.
Summary
Previously the
data/migrator
usedgorm.DB
to perform queries. This PR changes it to accept an interface that closely resembles the interface provided by the stdlib. The only difference from the stdlib is aDriverName
method. That extra method should be easy to implement by checking the type of the driver, or we may decide to remove sqlite support and remove the need for that method.This change is done first before converting the query functions because
migrator
is a separate package whichdata
depends on. So it has to be converted first.This PR also removes the
UseTransaction
option from the migrator. We were not using this option, and it made it more difficult to accept an interface. In the future if we want to use a transaction for applying migrations we can manage the transaction fromdata.NewDB
and it will behave the same as the transaction managed by the migrator itself.Best viewed by individual commit.
Related to #2415