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

Sort migration ids before writing to summary file #21

Merged
merged 1 commit into from
Jul 29, 2016

Conversation

kshepard
Copy link
Contributor

I encountered a problem when performing the following steps:

  • Use the example project, perform an mg init followed by a mg migrate
  • Delete the database file
  • Run another mg init followed by a migrate
  • The migrations failed to run, because the migrations summary file had the migrations listed in an incorrect order (M3, M2, M1) -- it would try to run M3 first and would fail.

This commit ensures that the migrations in the summary file are properly ordered and fixes the above issue.

@lastland
Copy link
Owner

Hi @kshepard,

Thanks for reporting this and submitting a pull request! However, there are a few things I would like to clarify:

  • You are not supposed to run mg init after mg migrate. The command should only be run the first time you start the migration or after a mg reset command.
  • Deleting the database file manually is NOT recommended for restarting the migrations. The best way to do it is via the mg reset command: it will reset the database as well as other related files (like the summary file and the generated source code).

Maybe I should try to make this explanation more visible somewhere.

// need to sort the migration ids before writing them to
// the Summary file, so they are read in and processed
// in the correct order.
val sortedIds = ids.sortWith(_.toInt < _.toInt)
Copy link
Owner

Choose a reason for hiding this comment

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

The identifier sortedIds is not used in anywhere but the next line. Maybe it's a good idea to chain it in the next line (like ids.sortWith(...).map(...))? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that sounds good to me, I'll update the PR.

@kshepard
Copy link
Contributor Author

Hi @lastland, thanks for taking a look at the PR!
The reason I was deleting the database file was to simulate a deployment scenario. I wanted to see what it would be like to develop the migrations locally, and then at a later time, change my database configuration and migrate a different, remote database. The database deletion was a simple way for me to test this out without needing to set up a completely separate database, but the results should be the same.

@kshepard kshepard force-pushed the feature/sort-migrations-summary branch from 6c74e91 to 6428778 Compare July 29, 2016 11:08
@lastland lastland merged commit e6e4364 into lastland:develop Jul 29, 2016
@lastland
Copy link
Owner

@kshepard Yeah that makes good sense. Thanks again for the PR!

@kshepard kshepard deleted the feature/sort-migrations-summary branch July 29, 2016 12:42
@kshepard
Copy link
Contributor Author

No problem, thanks for building this project!
Also, curious about your release cycle: approximately when do you next expect to publish a 0.2.2 release?

@lastland
Copy link
Owner

There has been quite a few changes so I plan to publish a 0.2.2 release soon -- hopefully tomorrow. Would that be OK for you?

@kshepard
Copy link
Contributor Author

Sounds wonderful, thank you!

@lastland
Copy link
Owner

lastland commented Jul 30, 2016

0.2.2 is released, though it may take a few hours before the release is synced to mvn repo center.

karolchmist pushed a commit to karolchmist/scala-forklift that referenced this pull request Apr 24, 2020
…summary

Sort migration ids before writing to summary file
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