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

Add versioned migrations #1313

Merged
merged 13 commits into from
Aug 29, 2024
Merged

Add versioned migrations #1313

merged 13 commits into from
Aug 29, 2024

Conversation

GAlexIHU
Copy link
Contributor

@GAlexIHU GAlexIHU commented Aug 6, 2024

Summary

  • We have to use atlas to generate the diffs based on the ent schema we declare
  • Atlas doesn't support multiple histories in a single database/schema (plain english: you can't change the lock table) so for migration running we have to use stg else (golang-migrate in this case)
  • Ent's Schema.Migrate is replaced by tool/migrate custom utility for running migrations as part of Application Startup. This behavior is configurable via PostgresConfig.AutoMigrate field/type.

Current Workflow

  1. Do your changes in openmeter/ent, then atlas migrate --env local diff <diffName> creates the new migration files
  2. You can apply your migrations with
migrate -database "postgres://postgres:postgres@localhost:5432/postgres?sslmode=disable&x-migrations-table=schema_om" -path ./tools/migrate/migrations up

Or you can set postgres.autoMigrate=migration to run migrations at startup

Other useful commands

# Lint migrations (atlas linting is quite good)
atlas migrate --env local lint
atlas migrate --env local-om lint # For Cloud only referencing the OM migrations

# Validate migrations directory
atlas migrate --env local validate

Added CI & Consistency Cheks

  • We validate that all code is generated (meaning the ent schema is in sync with the generated client)
  • We validate that the migrations directory is in sync with the schema
  • We run linting on the migrations directory to adhere guard against destructive changes
  • We validate migrations directory consistency via atlas checksum
  • We test that the migrations do run (up-down-up on an empty DB)

Future Improvements on Current Workflow

  • Using x-migrations-table in the connection string is a burden running manually. tools/migrate sets it consistently when invoking from code. A possible direction going forward could be to build on that custom tooling for both the local dev scripts as well as CI & Deploy automations (with all the drawbacks of having custom tooling for this)

Notes for Reviwers

  • There's a sister PR to this one, you can check it here
  • postgres.autoMigrate has a default value of ent which is same as the current default. However config.example.yaml promotes using the migration value

@GAlexIHU GAlexIHU added the release-note/misc Miscellaneous changes label Aug 6, 2024
@GAlexIHU GAlexIHU force-pushed the feat/migrations branch 4 times, most recently from 3838aaa to 3e787a8 Compare August 9, 2024 12:42
@GAlexIHU GAlexIHU requested review from sagikazarmark and turip August 9, 2024 14:27
@GAlexIHU GAlexIHU marked this pull request as ready for review August 9, 2024 14:33
@GAlexIHU GAlexIHU force-pushed the feat/migrations branch 8 times, most recently from 178786b to 465a6ae Compare August 27, 2024 12:52
turip
turip previously approved these changes Aug 27, 2024
Copy link
Contributor

@turip turip left a comment

Choose a reason for hiding this comment

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

I think you did a great job with this. 🙇

I would love if we could make the migration generation part of the make generate flow, just for ease of use. I suppose that's not done due to the naming requirement, right?

I would love if @sagikazarmark would also take a look, before merging.

ci/migrate.go Outdated Show resolved Hide resolved
chrisgacsal
chrisgacsal previously approved these changes Aug 28, 2024
Copy link
Contributor

@chrisgacsal chrisgacsal left a comment

Choose a reason for hiding this comment

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

Awesome job! Would love to have others also taking a look on it, but I am happy with the current solution.

feat: install atlas from dist binary with nix

fix: switch from community version of atlas

feat: define initial migration

chore: make automigrate configurable and add cmd for local

refactor: get rid of make commands and use atlas config file

refactor: use golang-migrate format

chore: add golang migrate to env

refactor: write custom migrate util

refactor: expose migrate interface

feat: add url parser utility

fix: enable migrations under custom path

fix: dont return error on no new migrations

fix: fix lint and test

fix: update e2e config

feat: support multiple automigrate modes
fix: fix e2e config

chore: regenerate init migration and add config test

test: add up down up test
GAlexIHU and others added 8 commits August 29, 2024 12:21
@GAlexIHU GAlexIHU merged commit 14bba4b into main Aug 29, 2024
18 checks passed
@GAlexIHU GAlexIHU deleted the feat/migrations branch August 29, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc Miscellaneous changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants