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: use flyway for db migrations #1965

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

feat: use flyway for db migrations #1965

wants to merge 3 commits into from

Conversation

yusefnapora
Copy link
Contributor

Hey everyone, as I was distracting myself from something else, I started looking into using Flyway for database migrations, using the node-flywaydb package to pull down and run the flyway cli.

This is related to #1692, but will need discussion before we can call the matter resolved. I just figured it would be easier to talk about if we had some code we could point to 😄

Anyway, here's what's going on in the PR. The db/migrations directory now has flyway-compatible migrations that set up the DB schema as it currently is in main.

It uses Flyway's placeholders to set the DAG_CARGO_* constants when setting up the foreign data wrapper.

It also makes the assumption that if DAG_CARGO_HOST and friends aren't in the environment, you want to use the testing dag-cargo config instead of the foreign data wrapper. To make that work, if the vars for the FDW aren't there it will set the placeholder DAG_CARGO_TEST_MODE to true, and the cargo-related migrations are wrapped in IF statements to run the test mode SQL if that var is true, and the FDW setup otherwise.

On reflection, that might be too much magic - maybe it's better to read DAG_CARGO_TEST_MODE from the environment instead, and always set it in our dev and test setups.

All the placeholder stuff is in db/flyway-config.cjs.

I haven't thought through integrating with CI yet, since I wanted to get feedback and make sure this is what we actually want, etc.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jun 6, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 922a592
Status: ✅  Deploy successful!
Preview URL: https://2e0b4fd0.nft-storage-1at.pages.dev

View logs

@yusefnapora
Copy link
Contributor Author

Stray observations about flyway:

Either of those could be used to set the MAINTENANCE_MODE flag for migrations that require disabling writes.

It doesn't seem to support automatic rollbacks, but in the paid version you can write "undo migrations" to reverse a versioned migration.

@codecov-commenter
Copy link

Codecov Report

Merging #1965 (922a592) into main (377b045) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main     #1965   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1259      1259           
=========================================
  Hits          1259      1259           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad5773c...922a592. Read the comment docs.

@vasco-santos
Copy link
Contributor

Flyway seems quite simple and a solution for us to consider. Would love to see, the alternative, how it would be if we execute scripts on a github action with a postgres script, almost like we do in cron jobs with manual trigger.

@yusefnapora
Copy link
Contributor Author

I'm curious about the custom alternative also. There are some things we'd like to do (like staging backup / rollback) that would be hard to fit into the Flyway scheme of things. It feels like Flyway wants to "own" the whole migration process, but will let you hook into it if you pay them 💸

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