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

Extended migration validation to guard against out-of-order migrations #3824

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pkt-zer0
Copy link

Currently, the migration list validation errors out if one of the completed migrations is missing. However, it's still possible to add new migrations before the completed ones.

This can produce inconsistent ordering in different databases, for example:

  • Run two migrations, named "1" and "3".
  • Add another migration and run it, named "2".
  • The resulting order will be "1, 3, 2".
  • Rollback all the migrations, and run them again.
  • The resulting order this time will be "1, 2, 3" (which would be expected).

To guard against this, there is an extra check to see if there are any migrations that would come before the latest completed one. If there are, an error is produced.

- Checks if a new migration appeared that's supposed to be earlier than the last completed one
@elhigu
Copy link
Member

elhigu commented Apr 23, 2020

I'm pretty sure we don't want to have strict checks that migrations has been ran in the same order. At least for us it has been common practise in development that each feature branch might contain migrations and it doesn't matter in which order those migrations are ran in the end.

In that development model both migrations has been designed to work separately and there is really no way to know in which order those migrations ends up to the master branch, so it is up to tests to make sure that everything works when migrations has been applied.

So if tests are added and this check is made optional this could be fine.

@pkt-zer0
Copy link
Author

The "disableMigrationListValidation" flag already turns this off. Or do you think a separate flag might be better, and have this be disabled by default? I'm not sure which is more common, expecting migrations to be ordered vs unordered.

As for testing: all right, I can give it a shot. There's an integration test for disableMigrationListValidation, I can do it similar to that easily. Dunno if that's the best approach, but it'll work.

@elhigu
Copy link
Member

elhigu commented Apr 24, 2020

In our use verifying that none of the migration files has been missing is useful to prevent filedeletion accidents.

I'm not sure which is more common, expecting migrations to be ordered vs unordered.

I would suggest that somewhat unordered is more common in projects where many people are developing different parts of the same application parallel in multiple branches (no one can know beforehand in which order features are merged to the master and in what exact timestamp migration file was created).

So my opinion is to not set order validation on by default for backwards compatibility and since I have never seen this as a problem (but as a feature) in our development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants