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

activerecord: Initialize Migration with version from MigrationProxy. #13355

Merged
merged 1 commit into from
Jan 7, 2014

Conversation

dylanahsmith
Copy link
Contributor

Problem

When running migrations with rake db:migrate, notice how the output to standard output looks like the following.

==  CreateUserTable: migrating ================================================
==  CreateUserTable: migrated (0.0000s) =======================================

This output is done through Migration#announce, delegated from MigrationProxy, as follows

From
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/migration.rb#L596

    def announce(message)
      text = "#{version} #{name}: #{message}"
      length = [0, 75 - text.length].max
      write "== %s %s" % [text, "=" * length]
    end

The output is supposed to include a version, but the version is always nil within the migration. The MigrationProxy has the version, but initializes the Migration with the default parameters (i.e. version = nil)

This missing version can also be a problem when trying to use the version within a migration. Our use case for doing this is to update a schema_migrations table on each shard, so a migration that succeeds on one shard, then fails on another, can be retried only on the shards it still needs to run on.

Solution

Initialize the Migration loaded from a MigrationProxy with the version.

Backport

This can be cherry-picked without conflict onto the 4-0-stable or 3-2-stable branch.

@carlosantoniodasilva
Copy link
Member

Any way to provide a test case so that we can be safe it won't happen again in the future? Thanks for the detailed explanation.

@dylanahsmith
Copy link
Contributor Author

The migration itself is not exposed on the MigrationProxy, so I just created a migration that raised if the version was incorrect as a test.

@@ -79,6 +79,12 @@ def test_migrator_versions
ActiveRecord::Migrator.migrations_paths = old_path
end

def test_migration_version
assert_nothing_raised do
Copy link
Member

Choose a reason for hiding this comment

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

We don't need the assert_nothing_raised. If something is raised the test will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the assert_nothing_raised

@arthurnn
Copy link
Member

arthurnn commented Jan 2, 2014

❤️ 💚 💜 💛
👍

@arthurnn
Copy link
Member

arthurnn commented Jan 2, 2014

@dylanahsmith Can you add a changelog for this?

@kuldeepaggarwal
Copy link
Contributor

👍

@steverob
Copy link

steverob commented Jan 3, 2014

Superb. 👍

@ghost ghost assigned rafaelfranca Jan 3, 2014
@dylanahsmith
Copy link
Contributor Author

Added an active record changelog entry.

rafaelfranca added a commit that referenced this pull request Jan 7, 2014
activerecord: Initialize Migration with version from MigrationProxy.

Conflicts:
	activerecord/CHANGELOG.md
@rafaelfranca rafaelfranca merged commit 06ace1e into rails:master Jan 7, 2014
rafaelfranca added a commit that referenced this pull request Jan 7, 2014
activerecord: Initialize Migration with version from MigrationProxy.

Conflicts:
	activerecord/CHANGELOG.md

Conflicts:
	activerecord/CHANGELOG.md
@dylanahsmith dylanahsmith deleted the migration-version branch January 7, 2014 20:52
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

6 participants