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

Allow sqlite migrations for non-default databases. #2610

Merged
merged 3 commits into from
Jul 22, 2024

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented Jul 1, 2024

Fixes #2607

The database will be selected by the name of the migration file. This makes the name of the migration file significant, whereas previously it was not. If the name of the file and a known database don't line up, spin up will fail with an error message.

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given we have docs that use examples like spin up --sqlite @migrations.sql, and that migrations may come as a sequence of files (a la migrations001.sql, migrations002.sql, etc.), I'm reluctant to adopt this UI. Let's instead look for a more structured and specific syntax, such as --sqlite custom-db:@migrations.sql or --sqlite @migrations.sql>custom-db (the latter would presumably make shells mad but just trying to spark ideas).

@rylev
Copy link
Collaborator Author

rylev commented Jul 2, 2024

@itowlson thanks for raising these concerns. I personally don't have strong feeling about what the right UX is here. I'd just want to ensure that it's easy to tell when the user is passing a file vs when they're passing raw SQL (since we support both). Right now the @ is used to disambiguate so perhaps we should keep the leading @. Perhaps a : between file name and databasename (i.e., @file.sql:database-name)?

@itowlson
Copy link
Contributor

itowlson commented Jul 2, 2024

@file.sql:database-name works for me. Thanks!

@rylev rylev requested a review from itowlson July 3, 2024 13:06
@rylev
Copy link
Collaborator Author

rylev commented Jul 3, 2024

@itowlson ok done!

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of non-blocking suggestions, but before merging could you double check that the indents around lines 76-78 are right in the real code? I'm assuming a GH artifact but good to be sure! Otherwise LGTM - thanks!

crates/trigger/src/runtime_config/sqlite.rs Outdated Show resolved Hide resolved
crates/trigger/src/runtime_config/sqlite.rs Show resolved Hide resolved
crates/trigger/src/runtime_config/sqlite.rs Outdated Show resolved Hide resolved
The database will be selected by the name of the migration file. This
makes the name of the migration file significant, whereas previously it
was not. If the name of the file and a known database don't line up,
`spin up` will fail with an error message.

Signed-off-by: Ryan Levick <[email protected]>
Signed-off-by: Ryan Levick <[email protected]>
@rylev rylev force-pushed the allow-migrations-in-non-default branch from a644710 to 096904e Compare July 22, 2024 12:25
@rylev rylev enabled auto-merge July 22, 2024 12:25
@rylev rylev merged commit c5784b8 into main Jul 22, 2024
17 checks passed
@rylev rylev deleted the allow-migrations-in-non-default branch September 9, 2024 11:19
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.

SQLITE error after update from 2.5.1 to 2.6.0
2 participants