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

Pare back default `index` option for the migration generator #23179

Merged
merged 1 commit into from Jan 24, 2016

Conversation

Projects
None yet
4 participants
@prathamesh-sonpatki
Member

prathamesh-sonpatki commented Jan 22, 2016

r? @sgrif cc @rafaelfranca

@vipulnsward

This comment has been minimized.

Member

vipulnsward commented Jan 22, 2016

I believe, this will need a CHANGELOG.

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jan 22, 2016

Yeah, I will add it also looking at failures from railties.

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:pare-back-default-index-option-to-references branch 2 times, most recently Jan 22, 2016

@prathamesh-sonpatki prathamesh-sonpatki changed the title from `references` will add index by default now, user can `index: false` to opt out to Pare back default `index` option for the migration generator Jan 22, 2016

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:pare-back-default-index-option-to-references branch Jan 22, 2016

end
end
end

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jan 22, 2016

Member

I removed these tests. Should I add them for checking index_is_not_added_for_references_association ?

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:pare-back-default-index-option-to-references branch Jan 23, 2016

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jan 23, 2016

Changelog added and build is green now.

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 24, 2016

r? @matthewd IIRC you were involved in the versioned migration changes. That's landed, correct? I'm not familiar enough with what's happening there to know if this only affects new migrations or if it affects old as well. If we haven't finished versioned migrations, and this is changing the behavior of existing migrations, I feel quite strongly that this should wait until 5.1

@rails-bot rails-bot assigned matthewd and unassigned sgrif Jan 24, 2016

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:pare-back-default-index-option-to-references branch Jan 24, 2016

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jan 24, 2016

@sgrif Yup, we have to update https://github.com/rails/rails/blob/master/activerecord/lib/active_record/migration/compatibility.rb to not create index by default unless specified for legacy migrations. I will work on it.

@matthewd

This comment has been minimized.

Member

matthewd commented Jan 24, 2016

@prathamesh-sonpatki matthewd@f87c457 may be helpful... I had the Actual Change, but hadn't gone back and chased it through the tests

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jan 24, 2016

Thanks, I will check it.

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:pare-back-default-index-option-to-references branch 2 times, most recently Jan 24, 2016

Pare back default `index` option for the migration generator
- Using `references` or `belongs_to` in migrations will always add index
  for the referenced column by default, without adding `index:true` option
  to generated migration file.
- Users can opt out of this by passing `index: false`.
- Legacy migrations won't be affected by this change. They will continue
  to run as they were before.
- Fixes #18146

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:pare-back-default-index-option-to-references branch to 909818b Jan 24, 2016

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jan 24, 2016

@matthewd Please review, took care of legacy migrations with your commit and added a test case for it.

matthewd added a commit that referenced this pull request Jan 24, 2016

Merge pull request #23179 from prathamesh-sonpatki/pare-back-default-…
…index-option-to-references

Pare back default `index` option for the migration generator

@matthewd matthewd merged commit 235c759 into rails:master Jan 24, 2016

1 check passed

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

This comment has been minimized.

Member

matthewd commented Jan 24, 2016

❤️

@prathamesh-sonpatki prathamesh-sonpatki deleted the prathamesh-sonpatki:pare-back-default-index-option-to-references branch Jan 24, 2016

CodingItWrong added a commit to CodingItWrong/rails that referenced this pull request Jul 16, 2016

Update references generation docs to exclude index
In rails#23179 the migration generator was changed to no longer output `index: true` for `references` migrations. This updates the migrations guide to remove `index: true` from relevant examples.

[ci skip]

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jul 16, 2016

Fix wrong test name
- Followup of rails#23179

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Aug 17, 2016

abicky added a commit to abicky/ridgepole that referenced this pull request Aug 19, 2017

Add an index to the referencing column by default
The default value of `index` is true from Rails 5.0.
cf. rails/rails#23179

abicky added a commit to abicky/ridgepole that referenced this pull request Aug 19, 2017

Add an index to the referencing column by default
The default value of `index` is true from Rails 5.0.
cf. rails/rails#23179

abicky added a commit to abicky/ridgepole that referenced this pull request Aug 19, 2017

Add an index to the referencing column by default
The default value of `index` is true since Rails 5.0.
cf. rails/rails#23179
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment