-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: sqlx migrations in test hooks #16
Conversation
8bef01a
to
65fdf06
Compare
65fdf06
to
6a8c36d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming together nicely! As our ITs evolve, I wonder if we want to advanced the structure now rather than later. I think there are a few bits that we should consider untangling. I'll share my thoughts below, but keen for your input as well.
So the setup I think we want goes something like this:
- We run the suite of ITs via a script and they setup/teardown an ephemeral stack. For example, this script spins up docker services, the ratings service, bootstraps the database and runs the tests. It uses .env and docker-compose files to avoid conflicting with locally running instances. This way we can run it locally before pushing and in our cicd pipeline and maybe a few cli switches to do things like "--no-bootstrap", "--no-teardown", etc. Eventually we'd snapshot the db and save as an artefact when in the pipeline, but I think for now its sufficient to simply not teardown as this facilitates manual inspection locally.
- When the ratings service starts it runs all migrations (ie inside
main.rs
). Doing this, ensures the database schema stays in sync across all envs (tests/local/stg/prd). - This ☝️ does not include 'bootstrap' because we expect individual envs to manage the bootstrapping (or not as will sometimes be the case for local and always be the case for stg and prd which I believe we want to do from the charm).
WDTY?
|
||
use sqlx::{postgres::PgPoolOptions, PgPool}; | ||
|
||
const MIGRATIONS_PATH: &str = "sql/migrations"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this means that clippy doesn't consider code invoked from /tests/
for this lint?
I think for now, we should opt to #[allow(lint_xyz)] to reduce noise. We can remove these allows as / when they're no longer needed. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, its strange. It's right that it isn't used in the bin module, but it is exposed by the lib, and used in tests. I'll tag them for now.
Feels like the right and natural progression. Its already getting quite fussy about how we set up the env locally, so feels like the time to move to using an ephemeral env 🙂 I will also move the migrator into main.rs 👍 I will push changes to ignore the lints on code used in /tests/ and merge so I can iterate on this code for the next stage of the IT setup. |
6a8c36d
to
7cb66cf
Compare
Changes:
INITIALIZATION_FLAG
from hooks. Was having some issues with threads yielding as strange times causing issues so Swapped to acall_once()
To test locally:
ratings
databasesql/bootstrap/roles.up.sql
sql/bootstrap/roles.up.sql
migrations_user
to your.env
file underAPP_MIGRATION_POSTGRES_URI
migrations_user
aligns with the user in your .env file underAPP_MIGRATION_POSTGRES_URI
cargo run
cargo test
We might want to look at moving when we load infrastructure and connect as the
service
db user. As it stands, theservice
db user is needed before the app starts, so can't wait formigrator.rs
to run to create it. Easiest thing might be to not evaluate the future for the service user connection until it is needed, delaying it to after the migrations have run?Clippy does complain about Migrator and its functions never being used even thought they are used in the testing module, so ignoring that for now.
Once we finalize these changes I'll append to the HACKING.md how to set up and run the tests locally 🙂