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 golang migrate #1504

Open
wants to merge 6 commits into
base: v3
Choose a base branch
from
Open

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Dec 19, 2024

Description

This PR removes our home-grown migration code in favour of golang-migrate.

Some reservations I had with Golang-migrate over Goose and why I'm okay with it now:

  • Migrations, by default, are not run in a transaction.
    • Thanks to Postgres and multi-statements, each migration file is run in a transaction. If we want to move to a different SQL DB or be more explicit we can add BEGIN/COMMIT statements to our transactions.
  • Large set of dependencies in the golang-migrate library.
    • After importing the library and the Postgres driver, we only have 4 new indirect dependencies so it doesn't seem too bad.
  • When a migration fails, the DB is marked as "dirty" and requires manual intervention.
    • Thanks to Postgres/transactions, we don't end up in an inconsistent state and when we detect this we can handle it in code. I've also added a test for this.

I recommend reviewing this PR commit-by-commit to more easily see the changes.

The benefits this gives us aren't huge. One benefit is that we don't need to modify the versions.go file when we add a new migration file, it will just automatically be applied, leaving less room for error.

Fixes JUJU-7047

Engineering checklist

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Test instructions

To test the new migrations on a fresh DB, just docker compose --profile dev up.

To test migrations on a DB that was using the old-style migrations. Switch to the v3 branch then ``docker compose --profile dev up` then switch to this branch and observe the jimm logs.

@kian99 kian99 requested a review from a team as a code owner December 19, 2024 07:03
@kian99 kian99 force-pushed the use-golang-migrate branch from a09ff0c to e38d434 Compare December 19, 2024 07:32
Copy link
Contributor

@SimoneDutto SimoneDutto left a comment

Choose a reason for hiding this comment

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

nice work, left some comments

docker-compose.yaml Show resolved Hide resolved
internal/db/db.go Outdated Show resolved Hide resolved
internal/db/db.go Show resolved Hide resolved
}
}

atomic.StoreUint32(&d.migrated, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we also check this flag at the start of the Migrate method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but we didn't before so I haven't changed that logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

i guess it's uncommon we call the Migrate twice in our tests, because we usually create a new db for each test.

Copy link
Contributor

@SimoneDutto SimoneDutto left a comment

Choose a reason for hiding this comment

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

lgtm

internal/db/db.go Outdated Show resolved Hide resolved
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