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

Add migration versioning via Migration subclasses #21538

Merged
merged 8 commits into from Dec 15, 2015

Conversation

Projects
None yet
7 participants
@matthewd
Member

matthewd commented Sep 8, 2015

This PR implements the migration versioning infrastructure, and restores the previous default on timestamps null: (when in 4.2-compatibility mode).

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Sep 8, 2015

r? @chancancode

(@rails-bot has picked a reviewer for you, use r? to override)

rails-bot commented Sep 8, 2015

r? @chancancode

(@rails-bot has picked a reviewer for you, use r? to override)

Show outdated Hide outdated activerecord/lib/active_record/migration.rb
def self.inherited(subclass) # :nodoc:
super
if subclass.superclass == Migration
subclass.send :include, Compatibility::Legacy

This comment has been minimized.

@artofhuman

artofhuman Sep 8, 2015

Contributor

mb use ? subclass.include Compatibility::Legacy

@artofhuman

artofhuman Sep 8, 2015

Contributor

mb use ? subclass.include Compatibility::Legacy

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Sep 9, 2015

Member

LGTM. I guess the main things to point out is the difference between upgrade strategies, namely this vs this. I think the strategy in this PR is easier. But, FWIW, both PRs generate a superclass, so we can switch to the strategy pattern in #21037 at pretty much any point if we need to.

The other thing is that this PR doesn't support deriving the version from the file name. IMO the syntax proposed here is pretty unoffensive, so I'm not sure if we still want / need to derive this from the file name.

I think the main hurdle here is that we need to figure out if the filename stuff is still required.

/cc @dhh @jeremy

Member

tenderlove commented Sep 9, 2015

LGTM. I guess the main things to point out is the difference between upgrade strategies, namely this vs this. I think the strategy in this PR is easier. But, FWIW, both PRs generate a superclass, so we can switch to the strategy pattern in #21037 at pretty much any point if we need to.

The other thing is that this PR doesn't support deriving the version from the file name. IMO the syntax proposed here is pretty unoffensive, so I'm not sure if we still want / need to derive this from the file name.

I think the main hurdle here is that we need to figure out if the filename stuff is still required.

/cc @dhh @jeremy

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Sep 9, 2015

Member

I'm happy enough with the superclass having the version identifier. Don't need the filename to include it too, imo. This is actually a pretty elegant solution.

Member

dhh commented Sep 9, 2015

I'm happy enough with the superclass having the version identifier. Don't need the filename to include it too, imo. This is actually a pretty elegant solution.

@matthewd matthewd changed the title from WIP: Add migration versioning via Migration subclasses to Add migration versioning via Migration subclasses Dec 15, 2015

@matthewd matthewd closed this Dec 15, 2015

@matthewd matthewd reopened this Dec 15, 2015

matthewd added some commits Dec 5, 2015

In 4.2 migrations, `timestamps` defaulted to `null: true`
.. it also showed a deprecation warning, but we obviously needn't retain
that.
Use a deliberately-invalid migration version in all doc examples
If we use a real version, at best that'll be an onerous update required
for each release; at worst, it will encourage users to write new
migrations against an older version than they're using.

The other option would be to leave these bare, without any version
specifier. But as that's just a variant spelling of "4.2", it would seem
to raise the same concerns as above.
Internal test migrations use the private 'Current' version
Apart from specific versioning support, our tests should focus on the
behaviour of whatever version they're accompanying, regardless of when
they were written.

Application code should *not* do this.
Use a real migration version number in docs
Even though this means more things to change when we bump after a
release, it's more important that our examples are directly copyable.

matthewd added a commit that referenced this pull request Dec 15, 2015

Merge pull request #21538 from matthewd/migration-version
Add migration versioning via Migration subclasses

@matthewd matthewd merged commit cd90f4f into rails:master Dec 15, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Dec 15, 2015

Member

🤘

Member

jeremy commented Dec 15, 2015

🤘

JuanitoFatas added a commit to JuanitoFatas/mailkick that referenced this pull request Feb 1, 2016

Update migration file for Rails 5
Rails 5 timestamp has `null: false` options.

Ref. rails/rails#16481

Add version identifier to superclass if on Rails 5

Ref. rails/rails#21538

JuanitoFatas added a commit to JuanitoFatas/mailkick that referenced this pull request Feb 1, 2016

Update migration file for Rails 5
Rails 4.2 timestamp needs to pass in `null: false` options.

Ref. rails/rails#16481

Add version identifier to superclass if on Rails 5

Ref. rails/rails#21538

@kenchan0130 kenchan0130 referenced this pull request May 14, 2017

Closed

Support rails5 for templates #243

1 of 1 task complete

@matthewd matthewd deleted the matthewd:migration-version branch Jun 11, 2017

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