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

Generate migrations at path set by `config.paths["db/migrate"]` #27674

Merged
merged 1 commit into from Jan 17, 2017

Conversation

Projects
None yet
6 participants
@kjg
Contributor

kjg commented Jan 13, 2017

Summary

Generate new database migrations at path set by config.paths["db/migrate"] instead of hardcoding the generations to db/migrate.

@kjg kjg force-pushed the kjg:migration_generator_honor_path_config branch Jan 13, 2017

@kjg kjg force-pushed the kjg:migration_generator_honor_path_config branch Jan 16, 2017

@kjg kjg force-pushed the kjg:migration_generator_honor_path_config branch to c942361 Jan 16, 2017

@kjg

This comment has been minimized.

Contributor

kjg commented Jan 16, 2017

@jeremy or @pixeltrix would you mind taking a quick look at this? Thanks!

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Jan 17, 2017

@kjg any reasons why you'd want to have migrations in a different place? Seems like a recipe for confusion in a project.

@kjg

This comment has been minimized.

Contributor

kjg commented Jan 17, 2017

@pixeltrix My use case is using activerecord via standalone-migrations to maintain db migrations/schemas for multiple applications all in one repo.

The location of migrations is already configurable

@migrations_paths ||= Rails.application.paths["db/migrate"].to_a

If we don't want it to be, maybe we should remove paths["db/migrate"] all together. If we're okay with keeping it configurable, let's make sure we're configurable everywhere that it matters.

@pixeltrix pixeltrix merged commit 4965fc1 into rails:master Jan 17, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kjg

This comment has been minimized.

Contributor

kjg commented Jan 17, 2017

Thanks! Should I make a PR against 5-0-stable as well? I'd personally like to see this in 5.0.x if possible.

migration = "migration_in_custom_path"
run_generator [migration]
Rails.application.config.paths["db/migrate"] = old_paths

This comment has been minimized.

@simi

simi Jan 17, 2017

Contributor

Will be nice to wrap this into ensure block to cleanup changed setting also when something fails during this test.

This comment has been minimized.

@pixeltrix

pixeltrix Jan 17, 2017

Member

@simi thanks - fixed in a0e0505

This comment has been minimized.

@simi

simi Jan 17, 2017

Contributor

Same problem is in model_generator_test. I just mentioned that only once here. Sorry :(

This comment has been minimized.

@pixeltrix

pixeltrix Jan 17, 2017

Member

@simi np - fixed in 33eef3f

This comment has been minimized.

@simi

simi Jan 17, 2017

Contributor

❤️

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Jan 17, 2017

@kjg backported in 3b2125e

@kjg kjg deleted the kjg:migration_generator_honor_path_config branch Jan 17, 2017

@rosenfeld

This comment has been minimized.

Contributor

rosenfeld commented Jan 21, 2017

This is awesome, thanks! I'll finally be able to get rid of this patch in active_record_migrations and fix it properly now.

@chris-roerig

This comment has been minimized.

chris-roerig commented Jan 21, 2017

This is great!

pixeltrix added a commit that referenced this pull request Mar 7, 2017

Check whether `Rails.application` defined before calling it
In #27674 we changed the migration generator to generate migrations at
the path defined in `Rails.application.config.paths` however the code
checked for the presence of the `Rails` constant but not the
`Rails.application` method which caused problems when using Active
Record and generators outside of the context of a Rails application.

Fixes #28325.

pixeltrix added a commit that referenced this pull request Mar 7, 2017

Check whether `Rails.application` defined before calling it
In #27674 we changed the migration generator to generate migrations at
the path defined in `Rails.application.config.paths` however the code
checked for the presence of the `Rails` constant but not the
`Rails.application` method which caused problems when using Active
Record and generators outside of the context of a Rails application.

Fixes #28325.

(cherry picked from commit 9b84567)

rosenfeld referenced this pull request in rosenfeld/active_record_migrations Mar 7, 2017

v5.0.2.1: should work with future versions of AR too
The migrations path was finally set as a separate method in the official
migration code since AR 5.0.2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment