Restore Migration.Up and Migration.Down signatures#1356
Open
bevzzz wants to merge 5 commits into
Open
Conversation
a5c7ae4 to
5f5b8cc
Compare
5f5b8cc to
293bb30
Compare
Collaborator
Author
|
@vmihailenco if possible, I'd like to include this in the next release, WDYT? Do you agree this is a breaking change worth fixing? |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. If there is no update within the next 7 days, this pr will be closed. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #1297
In an attempt to support templated SQL migration files #1188 changed the signature for
Migration.UpandMigration.Downfields, breaking a part of themigratepackage's public contract. This had the effect of breaking all external code that relied onMigrations.AddAPI previous to this change.My understanding is that
Migrations.Addwas never meant to be used outside of themigratepackage,Migrations.Registerbeing the officially documented and, I assume, the only intended way for client code to register its Go-based migrations. Based on the reactions to the linked issue, there seems to be a number of users who discovered Add (which is exported) and started using it.I propose we revert the breaking changes. Both
migrate.Migrationandmigrate.Migrations.Addhave been a part of the public API for a long time and breaking them isn't right. The reasoninternalMigrationFuncwas introduced was to support templated migrations and before/after hooks. For each of these the PR presents alternative, b/c-compatible solutions:context. This makes sense, because it is a piece of data scoped to a single migrate/rollback "request".UpandDownis responsible for applying the hooks, just like it is responsible for callingMarkApplied/MarkUnapplied.I also added the missing tests for these two features, see
testSQLMigrationsandtestHooks.