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

Fail migration with error if DB contains unknown migrations. #37

Merged

Conversation

mdales
Copy link
Contributor

@mdales mdales commented Jun 24, 2019

If gormigrate finds migrations in the DB table that aren't in the code provided list then it is likely the software is pointed at the wrong DB or somehow older code is running. Either way, we should not continue to work with this DB without human intervention.

I actually hit this in a production situation, and it was confusing as to why things were failing down the line (as the DB didn't match the model). I'd like to make it an explicit failure rather than something caught later.

Very happy to fix/improve things

If gormigrate finds migrations in the table that aren't in the provided
list then it is likely the software is pointed at the wrong DB or
somehow older code is running. Either way, we should not continue to
work with this DB without human intervention.
@mdales mdales force-pushed the mwd-error-on-unknown-migration branch from a712055 to 6746c35 Compare June 24, 2019 20:25
@andreynering
Copy link
Contributor

andreynering commented Jun 30, 2019

Hi @mdales,

Thanks for opening this pull request!

I have mixed feeling for this proposal. I agree that this may be useful for some people, but I think we should have a flag to enable this, and it should default to false.

There's two reasons for this:

  1. This change is backward incompatible: this would break stuff for people that already have old IDs in the database
  2. There are valid use cases for having old IDs in the database. Some people just deletes old migrations when they're sure all instance have already migrated. (New instance would use InitSchema if available)

@mdales
Copy link
Contributor Author

mdales commented Jun 30, 2019

Thanks for the feedback @andreynering, much appreciated. Happy to add the flag with an off default, will update the PR shortly (along with additional tests).

Copy link
Contributor

@andreynering andreynering left a comment

Choose a reason for hiding this comment

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

Hi again @mdales,

A few comments and we're good to go.

Thanks for writing tests! 😉

@andreynering andreynering merged commit 13b3726 into go-gormigrate:master Jul 7, 2019
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