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

Use a transaction to run data migrations #3157

Merged
merged 2 commits into from
Sep 26, 2022
Merged

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Sep 8, 2022

Summary

Also run each migration twice in the migration test to check for idempotence. Includes a couple fixes to migrations to make them idempotent.

internal/server/data/data.go Outdated Show resolved Hide resolved
internal/server/data/data.go Outdated Show resolved Hide resolved
@@ -79,6 +80,10 @@ func TestMigrations(t *testing.T) {
err := m.Migrate()
assert.NilError(t, err)

t.Run("run again to check idempotency", func(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

good idea 👍

@dnephin dnephin force-pushed the dnephin/use-txn-for-migration branch from db262a2 to 7e77d1d Compare September 9, 2022 17:40
To check for idempotency.

Also add HasFunction for checking if a function exists.
So that any failures are rolled back. We use a single transaction for
all migrations because on-prem deploys will likely want to keep the
old schema if any of the migrations in a new version fail.
@dnephin dnephin force-pushed the dnephin/use-txn-for-migration branch from 7e77d1d to a2da753 Compare September 21, 2022 17:43
Copy link
Contributor Author

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Ok, the other PRs have merged, so the TODO has been resolved.

This should be ready for a review

Copy link
Collaborator

@BruceMacD BruceMacD 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 to me

@dnephin dnephin merged commit 81c85bb into main Sep 26, 2022
@dnephin dnephin deleted the dnephin/use-txn-for-migration branch September 26, 2022 16:58
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.

2 participants