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

Simplify using schema migrations for JdbcSqliteDriver #3737

Merged
merged 14 commits into from
Jul 17, 2023
Merged

Simplify using schema migrations for JdbcSqliteDriver #3737

merged 14 commits into from
Jul 17, 2023

Conversation

morki
Copy link
Contributor

@morki morki commented Dec 14, 2022

See #2355, if this can be accepted, I will update documentation in this PR too.

@dellisd
Copy link
Collaborator

dellisd commented Dec 14, 2022

The JDBC driver probably isn't the place for this since PRAGMA is specific to SQLite. There'd need to be some other mechanism for other DBMS's like mysql or postgres, although I think people generally use dedicated migration systems for those kinds of databases (like flyway).

Maybe this would be better off as a separate module in :extensions which could be used with any of the SQLite drivers on any supported platform (JVM, Native, and JS).

@morki
Copy link
Contributor Author

morki commented Dec 14, 2022

@dellisd thank you for your response, it is extensions on JdbcSqliteDriver only, so this is for JDBC's Sqlite specialized implementation only. Other Sqlite drivers (native, JS) has their own way for migrations inbuilt too.

See NativeSqlDatabase.kt line 118 - 135

@dellisd
Copy link
Collaborator

dellisd commented Dec 14, 2022

Ah I didn't realize the native driver supported this already, thanks.

I don't think the JS driver does though, other than creating the schema without any checks (actually that sounds like a bug). But I see what you mean. It could be better to build this into the SQLite drivers (JVM/JS) since it's already supported on half of the platforms we target.

@morki
Copy link
Contributor Author

morki commented Dec 14, 2022

Yes, do you need anything from me in this? I don't know now what to do actually :)

@morki
Copy link
Contributor Author

morki commented Apr 27, 2023

@AlecStrong I updated to latest changes and applied your suggestion and updated documentation. Only thing I struggle with is test, can you please help?

@hfhbd
Copy link
Collaborator

hfhbd commented Jul 5, 2023

@morki What do you mean with test? Do you need help adding a test or do you mean the failing CI due using star imports? I fixed the latter to enable merging.

@morki
Copy link
Contributor Author

morki commented Jul 6, 2023

@hfhbd I thought adding unit test, if you think it is required.

@morki morki requested a review from AlecKazakova July 17, 2023 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants