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

refactor: migrate from sea-orm to sqlx #87

Merged
merged 11 commits into from
May 21, 2024
Merged

Conversation

vohoanglong0107
Copy link
Contributor

This is the first step to migrate from sea-orm to sqlx. All sea-orm models have been removed. All interaction with the postgres database through raw sql string is defined in module database/operations.
The tests are expected to fail in this PR, because all those test share the same database connection. A new PR will be raised later to fix those tests by using sqlx::test, allowing each test to have its own database.

Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Looks very good! Had a quick look and left a few comments.

migrations/20240517094752_create_build.down.sql Outdated Show resolved Hide resolved
src/database/mod.rs Outdated Show resolved Hide resolved
src/database/mod.rs Outdated Show resolved Hide resolved
src/github/mod.rs Outdated Show resolved Hide resolved
src/github/mod.rs Outdated Show resolved Hide resolved
@Kobzol
Copy link
Contributor

Kobzol commented May 20, 2024

LGTM, but we can't merge with tests failing. So either you need to comment out the tests in CI for now, or fix tests in this PR.

Btw, the Dockerfile needs updating, as it copies a directory that doesn't exist anymore.

@Kobzol
Copy link
Contributor

Kobzol commented May 20, 2024

To avoid having to commit the .sqlx directory created by cargo sqlx prepare, I think that the easier thing would be to spin up a Postgres instance in our CI tests and export a DATABASE_URL variable pointing to them.

It can be done with the services key: https://docs.github.com/en/actions/using-containerized-services/creating-postgresql-service-containers#running-jobs-in-containers

@vohoanglong0107
Copy link
Contributor Author

Got it. Once tried the approach of committing the .sqlx directory, I soon regreted it though.

@vohoanglong0107 vohoanglong0107 force-pushed the sqlx branch 2 times, most recently from 5e42c36 to b03d47b Compare May 20, 2024 13:26
Dockerfile Outdated Show resolved Hide resolved
@vohoanglong0107 vohoanglong0107 force-pushed the sqlx branch 5 times, most recently from 6e4d655 to 0be9ffe Compare May 21, 2024 00:09
@vohoanglong0107
Copy link
Contributor Author

vohoanglong0107 commented May 21, 2024

Since the tests fix is straightforward (by changing #[tokio::test] to #[sqlx::test] and handling the pool), I have also included the fix to this PR

Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Could you please also update docs/development.md? We should document the switch to sqlx and perhaps also prepare some guide for developers how to setup Postgres, i.e. with a simple docker-compose.yml file, and also the .env file so that compilation even works. Thanks!

.github/workflows/test.yml Show resolved Hide resolved
@vohoanglong0107
Copy link
Contributor Author

Do you think we should add a docker-compose.yaml file at the root of the project? That way we just need to add instructions on running the development environment instead of having to include the docker-compose.yaml content within the docs

@Kobzol
Copy link
Contributor

Kobzol commented May 21, 2024

Yeah, I also thought that it might be a good idea, to make it simpler for potential contributors to get the codebase working. While we're at it, I would also add the .env file (or at least .env.sample) with the connection string hardcoded for the Postgres configuration from the docker-compose.yml file.

@vohoanglong0107 vohoanglong0107 force-pushed the sqlx branch 2 times, most recently from 65848d8 to 3aeb20c Compare May 21, 2024 11:34
@vohoanglong0107 vohoanglong0107 requested a review from Kobzol May 21, 2024 11:34
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Good! I think that the only change left is to add a CI step that checks on CI that .sqlx is updated.

docs/development.md Outdated Show resolved Hide resolved
docs/development.md Outdated Show resolved Hide resolved
docs/development.md Show resolved Hide resolved
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Awesome! This looks really great, thanks a lot for the hard work! I think that now the DB side is much cleaner, and makes it easier for us to understand exactly what queries we are running against the DB.

@vohoanglong0107
Copy link
Contributor Author

vohoanglong0107 commented May 21, 2024

Haha, I have to admit, I was surprised that using raw sql string removed roughly 300 LOC from bors.

BTW, is there something wrong with the CI checks? It keeps being queued and hasn't even started yet.

@Kobzol
Copy link
Contributor

Kobzol commented May 21, 2024

Looks like some GHA bug (https://www.githubstatus.com/). The workflows have finished, but the status hasn't been updated yet.

@Kobzol Kobzol added this pull request to the merge queue May 21, 2024
@Kobzol Kobzol mentioned this pull request May 21, 2024
Merged via the queue into rust-lang:main with commit 8b7b835 May 21, 2024
2 checks passed
@vohoanglong0107 vohoanglong0107 deleted the sqlx branch May 22, 2024 00:47
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.

2 participants