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

Support for libsql and Turso #1042

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

avezina-ubik
Copy link

Resolves #1038

@bryanvaz
Copy link

bryanvaz commented Mar 9, 2024

@avezina-ubik Can you please correct the database connection uri in libsql/README.md and update your branch. The way your code is written, you have to specify the connection scheme after the libsql:// and does not require the query param. Specifically, your line:
database/libsql/README.md[5]:
libsql://[DATABASE].turso.io?authToken=[TOKEN]&query

Should read

libsql://https://[DATABASE].turso.io?authToken=[TOKEN]

In your unit tests you correctly insert the file:// scheme into the connection string after the libsql:// is stripped from the connection string which is why your tests pass.

Can you also add instructions on how to connect to a turso local dev/self-hosted server (e.g.: turso dev --db-file uki.db or via libsql Docker). On my local machine this is:

migrate -database libsql://http://localhost:8080

@dhui this might be another reason to move to individual CI tasks for each database. Then we can spin up sideband docker services and test cli commands against live local db instances to ensure the cli commands also work.

Cheers,
Bryan

Without the scheme in the connection string you will get the following error:

> migrate -source file://./migrations -verbose --database "libsql://rando-db-bryanvaz.turso.io?authToken=[TOKEN]&query" up
2024/03/09 18:12:13 error: unsupported URL scheme:
This driver supports only URLs that start with libsql://, file://, https://, http://, wss:// and ws://

while this works:

> migrate -source file://./migrations -verbose --database "libsql://https://rando-db-bryanvaz.turso.io?authToken=[TOKEN]" up
2024/03/09 18:15:09 Start buffering 20240309225149/u CreateCatCondoTable
2024/03/09 18:15:09 Read and execute 20240309225149/u CreateCatCondoTable
2024/03/09 18:15:09 Finished 20240309225149/u CreateCatCondoTable (read 79.775833ms, ran 122.719125ms)
2024/03/09 18:15:09 Finished after 219.338542ms
2024/03/09 18:15:09 Closing source and database

Copy link

@bryanvaz bryanvaz left a comment

Choose a reason for hiding this comment

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

update and test drop command - currently not working

return version, dirty, nil
}

func (d *LibSQL) Drop() (err error) {
Copy link

Choose a reason for hiding this comment

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

Please write a test for the drop command. It currently does not work:

❯ ./scripts/migrate drop
2024/03/09 18:46:33 Are you sure you want to drop the entire database schema? [y/N]
y
2024/03/09 18:46:34 Dropping the entire database schema
2024/03/09 18:46:34 error: failed to execute SQL: DROP TABLE sqlite_sequence
SQLite error: table sqlite_sequence may not be dropped in line 0: DROP TABLE sqlite_sequence in line 0: DROP TABLE sqlite_sequence

The following table is a libsql system table and should not be dropped:

  • sqlite_sequence

Additionally to cover most usecases:

  • Please drop views using DROP VIEW before you drop tables. This list can be obtained via: SELECT name FROM sqlite_master WHERE type = 'table'
  • The order of the tables in the sqlite_master is a hint as to the order in which they were created and thus any FK constraints; therefore you should drop the tables in reverse order to avoid hitting FK constraint issues.

- Fixed Drop failing when a table has AUTOINCREMENT
- Updated README.md to explain URL scheme
- Updated README.md to add instructions on how to connect to a turso dev
@avezina-ubik
Copy link
Author

Hey @bryanvaz!

Thanks for the review. I updated the README and tried to prioritez the libsql scheme since it's the one we mostly see in the Turso docs. My guess is it would be less confusing for users, but let me know what you think.

Fixed the Drop() too

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.

Support for libsql and Turso
2 participants