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

Refactor migration to move migrations paths to connection #31727

Merged
merged 1 commit into from Jan 18, 2018

Conversation

Projects
None yet
5 participants
@eileencodes
Member

eileencodes commented Jan 17, 2018

Rails has some support for multiple databases but it can be hard to
handle migrations with those. The easiest way to implement multiple
databases is to contain migrations into their own folder ("db/migrate"
for the primary db and "db/seconddb_migrate" for the second db). Without
this you would need to write code that allowed you to switch connections
in migrations. I can tell you from experience that is not a fun way to
implement multiple databases.

This refactoring is a pre-requisite for implementing other features
related to parallel testing and improved handling for multiple
databases.

The refactoring here moves the class methods from the Migrator class
into it's own new class MigrationContext. The goal was to move the
migrations_paths method off of the Migrator class and onto the
connection. This allows users to do the following in their
database.yml:

development:
  adapter: mysql2
  username: root
  password:

development_seconddb:
  adapter: mysql2
  username: root
  password:
  migrations_paths: "db/second_db_migrate"

Migrations for the seconddb can now be store in the
db/second_db_migrate directory. Migrations for the primary database
are stored in db/migrate".

The refactoring here drastically reduces the internal API for migrations
since we don't need to pass migrations_paths around to every single
method. Additionally this change does not require any Rails applications
to make changes unless they want to use the new public API. All of the
class methods from the Migrator class were nodoc'd except for the
migrations_paths and migrations_path getter/setters respectively.

cc/ @tenderlove

@eileencodes eileencodes self-assigned this Jan 17, 2018

@eileencodes eileencodes force-pushed the eileencodes:refactor-migration-classes-to-allow-for-migrations_paths-on-conn branch Jan 17, 2018

@tenderlove

LGTM. I think we can remove one more migrations_paths parameter, but this is good to merge as-is IMO.

activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb Outdated
@@ -1049,8 +1049,8 @@ def assume_migrated_upto_version(version, migrations_paths)
sm_table = quote_table_name(ActiveRecord::SchemaMigration.table_name)
migrated = ActiveRecord::SchemaMigration.all_versions.map(&:to_i)
versions = ActiveRecord::Migrator.migration_files(migrations_paths).map do |file|
ActiveRecord::Migrator.parse_migration_filename(file).first.to_i
versions = migration_context.migration_files(migrations_paths).map do |file|

This comment has been minimized.

@tenderlove

tenderlove Jan 17, 2018

Member

Can we eliminate the migration_paths parameter here? I think migration_context contains the same information.

This comment has been minimized.

@eileencodes

eileencodes Jan 17, 2018

Member

Doh I missed that one.

@eileencodes eileencodes force-pushed the eileencodes:refactor-migration-classes-to-allow-for-migrations_paths-on-conn branch 2 times, most recently Jan 17, 2018

@eileencodes eileencodes force-pushed the eileencodes:refactor-migration-classes-to-allow-for-migrations_paths-on-conn branch Jan 17, 2018

Refactor migration to move migrations paths to connection
Rails has some support for multiple databases but it can be hard to
handle migrations with those. The easiest way to implement multiple
databases is to contain migrations into their own folder ("db/migrate"
for the primary db and "db/seconddb_migrate" for the second db). Without
this you would need to write code that allowed you to switch connections
in migrations. I can tell you from experience that is not a fun way to
implement multiple databases.

This refactoring is a pre-requisite for implementing other features
related to parallel testing and improved handling for multiple
databases.

The refactoring here moves the class methods from the `Migrator` class
into it's own new class `MigrationContext`. The goal was to move the
`migrations_paths` method off of the `Migrator` class and onto the
connection. This allows users to do the following in their
`database.yml`:

```
development:
  adapter: mysql2
  username: root
  password:

development_seconddb:
  adapter: mysql2
  username: root
  password:
  migrations_paths: "db/second_db_migrate"
```

Migrations for the `seconddb` can now be store in the
`db/second_db_migrate` directory. Migrations for the primary database
are stored in `db/migrate`".

The refactoring here drastically reduces the internal API for migrations
since we don't need to pass `migrations_paths` around to every single
method. Additionally this change does not require any Rails applications
to make changes unless they want to use the new public API. All of the
class methods from the `Migrator` class were `nodoc`'d except for the
`migrations_paths` and `migrations_path` getter/setters respectively.

@eileencodes eileencodes force-pushed the eileencodes:refactor-migration-classes-to-allow-for-migrations_paths-on-conn branch to a2827ec Jan 18, 2018

@eileencodes eileencodes merged commit 6e74e1d into rails:master Jan 18, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate All good!
Details

@eileencodes eileencodes deleted the eileencodes:refactor-migration-classes-to-allow-for-migrations_paths-on-conn branch Jan 18, 2018

kamipo added a commit that referenced this pull request Jan 19, 2018

Remove unused `migration_context` in `DatabaseTasks`
This was added in #31727, but it is unused.
@andrew-aladev

This comment has been minimized.

andrew-aladev commented Feb 1, 2018

@matthewd, this PR is not related to maintaining multiple connections for multiple databases. Migrations are still working with ActiveRecord::Base.connection. They are applied one-by-one. It is not possible to apply migration when two connections are running.

@phildionne phildionne referenced this pull request Apr 26, 2018

Closed

Engine support #12

@bogdan

This comment has been minimized.

Contributor

bogdan commented Jun 6, 2018

I know this is too late, but maybe it will be helpful.
The convention explained here is weird:

development:
  adapter: mysql2
  username: root
  password:

development_seconddb:
  adapter: mysql2
  username: root
  password:
  migrations_paths: "db/second_db_migrate"

I'd have to duplicate migrations_path between different envs and the <env>_<db-name> convention feels redundant to me.
I would prefer the following format instead:

main:
  migrations_path: 'db/migrate'
  development:
    adapter: mysql2
    username: root
    password:
  test:
    ...
  production
    ...
seconddb:
  migrations_paths: "db/second_db_migrate"
  development:
    adapter: mysql2
    username: root
    password:
  test:
    ....
  production:
    ...
@eileencodes

This comment has been minimized.

Member

eileencodes commented Jun 7, 2018

@bogdan - it was just an example before I realized there was a three tier config. Both work.

davinlagerroos added a commit to umn-asr/iron_fixture_extractor that referenced this pull request Jun 14, 2018

Add support for running tests > ActiveRecord 5.2
The API for invoking migrations has changed in rails/active record 5.2
see rails/rails#31727

davinlagerroos added a commit to umn-asr/iron_fixture_extractor that referenced this pull request Jun 14, 2018

Add support for running tests > ActiveRecord 5.2
The API for invoking migrations has changed in rails/active record 5.2
see rails/rails#31727

Leave support for old migration API so we can run tests against old
versions of ActiveRecord to check compatibility

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 17, 2018

Fix deprecation warning of `ActiveRecord::Migrator.migrations_path=`
`ActiveRecord::Migrator.migrations_path=` was deprecated in rails#31727.

This commit fixes:
- `ActiveRecord::Migrator.migrations_path=` is deprecated, but not
`ActiveRecord::Migrator.migrations_paths=`
- Adds missing spaces
- Adds info that user can use `ActiveRecord::Migrator.migrations_paths=`
instead of `ActiveRecord::Migrator.migrations_path=`

The warning including this commit:
```
DEPRECATION WARNING: `ActiveRecord::Migrator.migrations_path=` is now
deprecated and will be removed in Rails 6.0. You can set the `migrations_paths`
on the `connection` instead through the `database.yml`, or
use `ActiveRecord::Migrator.migrations_paths=`.
```

Related to rails#31727

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 17, 2018

Fix deprecation warning of `ActiveRecord::Migrator.migrations_path=`
`ActiveRecord::Migrator.migrations_path=` was deprecated in rails#31727.

This commit fixes:
- `ActiveRecord::Migrator.migrations_path=` is deprecated, but not
`ActiveRecord::Migrator.migrations_paths=`
- Adds missing space

The warning including this commit:
```
DEPRECATION WARNING: `ActiveRecord::Migrator.migrations_path=` is now
deprecated and will be removed in Rails 6.0. You can set the `migrations_paths`
on the `connection` instead through the `database.yml`.
```

Since it was deprecated in Rails 5.2 we should backport it to the `5-2-stable` branch.

Related to rails#31727
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment