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

Allow to run migrations in check mode (dry run) #31630

Closed
wants to merge 1 commit into from

Conversation

fatkodima
Copy link
Member

I have found myself a lot in situations where it could be great to have ability to run migrations to report what changes they would have made rather than making them.

Of course, this can be done by running rake db:migrate:status to find targeted migrations, inspect their code for changes and mentally assemble those pieces in one flow. But this is tedious, slightly time consuming and error prone even for 3 migrations. And can be automated.

DRY_RUN option was added to db:migrate, db:rollback, db:migrate:up and db:migrate:down. For db:migrate:up and db:migrate:down it is not very much needed, but was added for consistency and to avoid possible surprises.

The output in check mode looks like this:

▶ rake db:migrate DRY_RUN=true
== 20171231134346 CreateActiveStorageTables: migrating ========================
-- create_table(:active_storage_blobs)
-- create_table(:active_storage_attachments)
== 20171231134346 CreateActiveStorageTables: migrated =========================

== 20180103151354 CreateUsers: migrating ======================================
-- create_table(:users)
== 20180103151354 CreateUsers: migrated =======================================

== 20180103151422 CreatePosts: migrating ======================================
-- create_table(:posts)
== 20180103151422 CreatePosts: migrated =======================================

== 20180103151456 AddEmailToUsers: migrating ==================================
-- add_column(:users, :email, :string)
== 20180103151456 AddEmailToUsers: migrated ===================================

If this feature will be ok for core members, I'll finish this PR by adding tests, documentation, etc.
Feedback would be greatly appreciated.

@javierhonduco
Copy link
Contributor

javierhonduco commented Jan 12, 2018

Damn, just saw this line

If this feature will be ok for core members, I'll finish this PR by adding tests, documentation, etc.
Feedback would be greatly appreciated.

BTW, deleted my two small code comments as they are in line with the way the rails codebase is written.

@matthewd
Copy link
Member

It seems dangerous to offer a dry-run mode when we're executing arbitrary user-supplied code: we can neuter the standard migration methods, but who knows what else someone might be doing inside their migrations. At best, it'll likely fail because preceding migrations haven't put the DB into the expected state; at worst, it could make real changes to data. 😕

@fatkodima
Copy link
Member Author

fatkodima commented Jan 18, 2018

@matthewd yes, very reasonable comment!
Looks like we can't neuter all possible code user would like to execute inside his migrations (at least not directly related for db migrations).
Wdyt, should this be closed or maybe there is a possible solution to that problem that I'm missing?

@javierhonduco
Copy link
Contributor

I think the only semi-safe way would be parsing those migration files and checking that its AST only has calls to ActiveRecord's migrations methods. This would probably not play well with other ways of running migrations that could be legit, so wouldn't work 100%, unfortunately.

@rafaelfranca
Copy link
Member

Given the already pointed problems and the fact the need for this is only for debug proposes I prefer to not add this complexity to the framework. Thank you so much for the pull request.

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.

None yet

4 participants