Skip to content

Add new option to run down migrations based on run order rather than creation order. #178

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

Merged
merged 2 commits into from
Aug 1, 2014

Conversation

ajkerr
Copy link
Contributor

@ajkerr ajkerr commented Jul 14, 2014

No description provided.

@kunklejr
Copy link
Contributor

Is this feature for a case where multiple developers have run migrations they've independently created against a shared database? It seems like this would be a bad situation to be in, no?

@ajkerr
Copy link
Contributor Author

ajkerr commented Jul 21, 2014

Not quite. This solves the case where migrations are created in some code branch, but are not actually run against the database until some future date, for example when a feature is ready to be deployed.

Because the name of the migration always includes the timestamp of its creation date, migrations could be rolled back in a different order from which they were run. This pull request allows you dictate that migrations are rolled back in the same order that they were run, rather than the order in which they were created.

@leedm777
Copy link
Contributor

+1 We've just run into a similar situation in our staging system.

Because of work that may happen in branches, the order in which migrations were applied is not necessarily the order in which they were created. But when you hit a situation where you want to migrate down (like downgrading something that was deployed to a staging environment), you really want the migrations run in the order of application.

@kunklejr
Copy link
Contributor

Perhaps that should just be the default behavior. Can anyone think of a reason you wouldn't want to run the down migrations in the order they were applied?

@ajkerr
Copy link
Contributor Author

ajkerr commented Jul 22, 2014

A good reason to keep this new behaviour optional is that some databases don't support fractional seconds in datetime fields. For example, MySQL didn't support this until version 5.6.4.

Using the run_on column could offer less precision than the existing method (using the migration names).

@leedm777
Copy link
Contributor

@ajkerr The patch accounts for that possibility. It orders the migrations by 'run_on DESC, name DESC', which should run migrations with the same 'run_on' time in the proper order.

+1 to making this the default behavior.

@ajkerr
Copy link
Contributor Author

ajkerr commented Jul 23, 2014

@leedm777 You're right - adding the secondary sort column was my attempt to get it to work correctly "most of the time", but it's still possible that the actual run order is not accurately reflected.

As an example, suppose I have two migrations:

  1. 20140625122730-migration-1
  2. 20140627171904-migration-2

Developer 1 runs migration 2 at 2014-07-23 06:00:00.443.
Developer 2 runs migration 1 at 2014-07-23 06:00:00.888.

The migrations table uses a vanilla DATETIME column, so the fractional seconds get truncated. If you then try to roll back the migrations using run order, they will be rolled back in the wrong order.

Admittedly, this is a contrived example, but it is still possible.

@leedm777
Copy link
Contributor

@ajkerr You're right, it's technically possible. But I still think that the proposed new behavior is right more often than it's wrong. I also expect that you don't often have different developers running different migrations on the same database at the same time.

@ajkerr
Copy link
Contributor Author

ajkerr commented Jul 23, 2014

@leedm777 I'm fine with updating the pull request to make this option the default if @kunklejr agrees.

@kunklejr
Copy link
Contributor

I agree with making it the default (and only) way of running it. An updated pull request would be greatly appreciated.

@ajkerr
Copy link
Contributor Author

ajkerr commented Jul 24, 2014

Pull request has been updated. I also took the liberty of removing a few unused functions.

@kunklejr kunklejr merged commit 4a9d343 into db-migrate:master Aug 1, 2014
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