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

Document best practices with old migrations #33474

Merged
merged 1 commit into from
Aug 11, 2018

Conversation

olivierlacan
Copy link
Contributor

The copy here is of course up for discussion but it feels like we need
to address the issue of old migrations in the Migration guide because
other than mentioning the canonical nature of schema.rb/structure.sql
or the actual database compared to migration files, it seems like more
guidance would help.

Here's a sample of the kinds of question people seem to often ask about
old Rails migrations:

The common theme seems to be: "I've got old migrations, should I keep
them around on an old project?".

My personal stance is that as long as migrations run and don't take too long do
so, you should keep them around since it allows people working on the Rails
project with you to seamlessly upgrade their local development database
without having to do a db:drop db:schema:load and lose all their seed data.

While writing down this suggested new section it felt like I was describing a
very cumbersome process that could be address with a rake task like:

rails db:migrate:remove VERSION=20121201123456

It rollback to the version just before 20121201123456, delete the migration
file, and run db:migrate to get back to the latest migration.

This of course doesn't address a situation when someone would want to delete or
merge all migrations prior to a certain date, which is addressed by
squasher.

I'm not sure this is something we want to encourage people to do. Although I
feel like with more and more production Rails apps over 5-years old, it's
definitely a concern we should address.

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@olivierlacan olivierlacan force-pushed the old-migrations branch 3 times, most recently from 73b4d37 to e3f8007 Compare July 30, 2018 15:06
`rails db:migrate` once more to re-run any migrations that followed the
now-deleted migration.


Copy link
Member

Choose a reason for hiding this comment

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

Can you ✂️ these 2 extra lines?


Since the `schema.rb` or `structure.sql` is an automatically generated snapshot
of the current state of the database it's possible — although not necessary
— to delete old migrations files.
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of this instead?

The schema.rb or structure.sql is a snapshot of the current state of your database
and is the authoritative source for rebuilding that database. This makes it possible to delete
old migration files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used this and updated the rest to avoid suggesting the convoluted flow that didn't seem necessary. Thanks!

@olivierlacan
Copy link
Contributor Author

@eileencodes Talking with @matthewd on Campfire it seems like this isn't really a process we'd recommend since there's no clear problems with deleting migration files other than having NO FILE show up in rails db:migrate:status for a migration timestamp in the schema_migrations which no longer has a corresponding file on disk.

I think I'm going to rewrite this to simplify the advice as:

You can delete old migrations, but remember that the schema_migrations table will still have a reference to them.

The copy here is of course up for discussion but it feels like we need
to address the issue of old migrations in the Migration guide because
other than mentioning the canonical nature of schema.rb/structure.sql
or the actual database compared to migration files, it seems like more
guidance would help.

Here's a sample of the kinds of question people seem to often ask about
old Rails migrations:

- https://stackoverflow.com/questions/20119391/delete-old-migrations-files-in-a-rails-app
- https://www.reddit.com/r/rails/comments/4ayosd/compacting_migrations_files_or_delete_them/
- https://stackoverflow.com/questions/4248682/is-it-a-good-idea-to-purge-old-rails-migration-files
- https://stackoverflow.com/questions/707013/is-it-a-good-idea-to-collapse-old-rails-migrations
- https://stackoverflow.com/questions/1981777/rails-remove-old-models-with-migrations
- https://stackoverflow.com/questions/3343534/rebase-rails-migrations-in-a-long-running-project

The common theme seems to be: "I've got old migrations, should I keep
them around on an old project?".

My personal stance is that as long as migrations run and don't take too long do
so, you should keep them around since it allows people working on the Rails
project with you to seamlessly upgrade their local development database
without having to do a `db:drop db:schema:load` and lose all their seed data.

While writing down this suggested new section it felt like I was describing a
very cumbersome process that could be address with a rake task like:

```bash
rails db:migrate:remove VERSION=20121201123456
```

It rollback to the version just before `20121201123456`, delete the migration
file, and run `db:migrate` to get back to the latest migration.

This of course doesn't address a situation when someone would want to delete or
merge all migrations prior to a certain date, which is addressed by
[squasher](https://github.com/jalkoby/squasher).

I'm not sure this is something we want to encourage people to do. Although I
feel like with more and more production Rails apps over 5-years old, it's
definitely a concern we should address.
@olivierlacan
Copy link
Contributor Author

@matthewd Let me know if the copy seems reasonable to you now.

@matthewd
Copy link
Member

matthewd commented Aug 3, 2018

Immediate thoughts are

  1. it might be worth adding an "and that's okay" to the new bit about missing files -- i.e., that while that message will show up, it's not alarming, and as long as you're expecting it, doesn't indicate anything bad; and
  2. if we're going to acknowledge that deleting the files is reasonable, we should perhaps include a warning to be sure the migration has been run on all extant environments: the big danger in deleting migrations is that if you delete a middle migration that has been run in some places but not others, you will end up with two databases that both think they're up to date, but are structurally different.

@schneems
Copy link
Member

I agree with MD. I also think this is a good place to start. Thanks for the PR!

@schneems schneems merged commit bddab1f into rails:master Aug 11, 2018
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Aug 12, 2018
- Name rails app' files relatively to its root
  - `structure.sql` =>  `db/structure.sql`
  - `schema.rb` =>  `db/schema.rb`
- Clarify rails commands
  - `db:migrate` => `rails db:migrate`
  - `db:migrate:status` => `rails db:migrate:status`
- Add `/` to the end of `db/migrate` in order to express that it is
  directory and to keep consistency with `db/migrate/` above.

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

Successfully merging this pull request may close these issues.

None yet

6 participants