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

feat: db migrations #533

Merged
merged 8 commits into from
Sep 16, 2024
Merged

feat: db migrations #533

merged 8 commits into from
Sep 16, 2024

Conversation

cylewitruk
Copy link
Member

@cylewitruk cylewitruk commented Sep 16, 2024

Description

Introduces db-migration functionality for the signer binary.

Note 1: I did first try using sqlx's migration functionality, but it pulls in dependencies which conflict with stacks-core deps. This seems to be documented as a known Cargo issue here. But in any case, handling them manually gives us a few more options, especially re: printing out pending migrations via a command line command.

Note 2: I took the db-migrations "as-is" for now. When in a user environment we may want to re-think the two scripts for creating the schema and user as depending on a company's policies these operations may not be allowed.

Question on Note 2: Is there any particular reason why we felt we needed to use a separate schema from public? I'm going under the assumption that we should require that the sBTC signer has its own dedicated database and in the case of using pgsql the user will probably create their own user and give it appropriate permissions (as designated by us). I otherwise have a hard time envisioning that the signer will grow to a size where schema isolation is desired/required?

Partially Closes: #532 (printing out pending db migrations is not in this PR)

Changes

  • New dependency on include_dir (which is similar to include_bytes etc. but for entire directories) which compiles the database migrations into the signer binary.
  • New optional command line flag --migrate-db which if set will attempt to apply any pending database migrations on signer startup. Default is false.
  • new_test_database() is updated with a flag for whether or not migrations should be applied. I had this originally for writing integration tests for the migrations themselves, but then decided that the migrations are implicitly tested for any test using this function so it wasn't explicitly needed. I left this flag in as it can be good for adding some negative integration tests (i.e. for when a user isn't using auto-migrations and has forgotten to apply migrations manually, etc.).
  • PgStore creates a new table public.__sbtc_migrations (I am assuming here that sBTC will have its own, dedicated db and that we can freely use public) for keeping track of applied migrations.
  • Changed the flyway service entry in docker-compose.test.yml to not run automatically in favor of db migrations being applied by code. If you use this compose file for local testing then you can still run it, just up flyway manually.
  • Changed the integration-test-full target to down the test env first (I ran into some issues here when it was lingering).

Testing Information

  • Integration tests have been updated and all tests pass.
  • This also adds a new docker-compose for running the signer locally with a db on a random port (need to fix bitcoin to do the same), with migrations enabled.

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@cylewitruk cylewitruk self-assigned this Sep 16, 2024
@cylewitruk cylewitruk added the sbtc signer binary The sBTC Bootstrap Signer. label Sep 16, 2024
Copy link
Contributor

@djordon djordon left a comment

Choose a reason for hiding this comment

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

My main question is about whether we should outsource the db migrations code to the another crate, but in general this seems fine.

signer/src/storage/postgres.rs Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
docker-compose.signer.yml Outdated Show resolved Hide resolved
signer/Cargo.toml Outdated Show resolved Hide resolved
signer/src/storage/postgres.rs Outdated Show resolved Hide resolved
signer/src/storage/postgres.rs Show resolved Hide resolved
signer/src/storage/postgres.rs Show resolved Hide resolved
Makefile Show resolved Hide resolved
Copy link
Contributor

@djordon djordon left a comment

Choose a reason for hiding this comment

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

Looks good

Makefile Show resolved Hide resolved
@cylewitruk
Copy link
Member Author

cylewitruk commented Sep 16, 2024

Looks good

Added #537 to revisit this pre-launch, and added a TODO comment in apply_migrations() referencing it as well.

@cylewitruk cylewitruk merged commit 50fc9fa into main Sep 16, 2024
3 checks passed
@cylewitruk cylewitruk deleted the feat/db-migrations branch September 16, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sbtc signer binary The sBTC Bootstrap Signer.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Feature]: Provide database migration mechanism for signer
2 participants