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

Enable create database #243

Merged
merged 5 commits into from
Apr 13, 2020

Conversation

JesperAxelsson
Copy link
Contributor

@JesperAxelsson JesperAxelsson commented Apr 10, 2020

So I did some clean up and integrated anyhow for cleaner error messages.
I also included a function that uses the connection string to create a database if there is none with the correct name in the database. Would like some feedback on wether that is a good idea or not.

As is you can't actually create the database in a normal migration because the connection will fail because there is no database...
This could be worked around in a number of ways.

  1. We could assume there is always a database created
  2. There could be specialized migration script that creates the database (like create_db.sql)
  3. Assume the first migration always is a create database script
  4. Use the connection string to create a database
  5. A mix of the above

I can see pros and cons with all of these methods. This PR implements nr 4.

ps. Also fixed so you can create two tables in the same migration.

@mehcode
Copy link
Member

mehcode commented Apr 10, 2020

Generally I see 1 in migration tools. However these are also tools following a pattern where the tool itself is intended to run in production and implicitly creating the database might not be wanted.

Hopefully, someone using SQLx in production is accessing the database using a user that doesn't have the ability to randomly make databases.

Of course if we treat cargo sqlx like a development tool, and I would like to, I like the idea of implicit database creation so my vote is for 4 like you have it setup.


Now when we get to the point where we like what cargo sqlx _ is doing and we move the core of the functionality into sqlx::migrate::_ I would be against that creating databases.

@D1plo1d
Copy link
Contributor

D1plo1d commented Apr 11, 2020

Now when we get to the point where we like what cargo sqlx _ is doing and we move the core of the functionality into sqlx::migrate::_ I would be against that creating databases.

Hoping to not derail this PR too badly (I also like option 4). I'm excited that a sqlx::migrate API could help the CI and cross compilation stories for slqx (#60 ). The way it plays out in my head is if I set up an empty database in my CI/rust cross docker image then a sqlx::migrate API would make it quite ergonomic to run all migrations in build.rs to give query! a database to build against (in CI and cross compilation - ie. docker containers where I don't have a dev or production database).

@JesperAxelsson
Copy link
Contributor Author

It might be better to just move the create db part to it's own command. Then a user could get a warning that the database is missing and either use the tool to create it or set it up themselves. Less magic :)

@mehcode
Copy link
Member

mehcode commented Apr 12, 2020

#170 (comment)

Great minds and all that .. haha.

@JesperAxelsson
Copy link
Contributor Author

#170 (comment)

Great minds and all that .. haha.

Was cheating a bit as I were answering that issue when I wrote that ^^

@JesperAxelsson
Copy link
Contributor Author

I moved the db creation to a command as in we talked about in #170
Database is not automatically created.

@mehcode mehcode merged commit c6e9b27 into launchbadge:master Apr 13, 2020
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.

3 participants