Change migration name to avoid conflicts #7526
Conversation
arpitchauhan
force-pushed
the
bugfix/migration-name
branch
from
15b0f1d
to
ce40ed9
Compare
|
Hi @arpitchauhan could you please rebase branch and create PR for |
|
@krzysiek1507 Rebasing is proving quite complicated (many conflicts). Is it ok if I cherry-pick my commit onto a new branch made out of master? |
|
@arpitchauhan sorry. I mean, rebase your branch with |
|
Sure, will do.
|
arpitchauhan
force-pushed
the
bugfix/migration-name
branch
from
ce40ed9
to
73a2a98
Compare
|
@krzysiek1507 Done. The other PR is #7530 |
arpitchauhan
force-pushed
the
bugfix/migration-name
branch
from
73a2a98
to
207d2fa
Compare
|
Hi @arpitchauhan could you add checking if indexes exist? |
|
@krzysiek1507 do you mean adding spec examples (tests)? |
|
@arpitchauhan I mean this. Because migration's name is changed, it will be copied and run, so this will cause problems. |
|
@krzysiek1507 Ah, got you. Will add them. |
| unless index_exists?(:spree_option_values_variants, [:option_value_id, :variant_id]) | ||
| add_index :spree_option_values_variants, | ||
| [:option_value_id, :variant_id], | ||
| name: 'index_option_values_variants_on_option_value_and_variant' |
There was a problem hiding this comment.
Align the parameters of a method call if they span more than one line.
arpitchauhan
force-pushed
the
bugfix/migration-name
branch
2 times, most recently
from
8b84bdc
to
4e011a2
Compare
|
@krzysiek1507 done |
|
@arpitchauhan could you squash commits please? |
arpitchauhan
force-pushed
the
bugfix/migration-name
branch
from
4e011a2
to
95d3bc6
Compare
|
@krzysiek1507 done |
| class AddMissingIndexesOnSpreeTables < ActiveRecord::Migration | ||
| def change | ||
| unless index_exists?(:spree_promotion_rules_users, [:user_id, :promotion_rule_id]) | ||
| add_index :spree_promotion_rules_users, |
There was a problem hiding this comment.
Shouldn't we consider index name in index_exists?? And please fix indentation.
There was a problem hiding this comment.
@krzysiek1507 The documentation says we need to use table name and column name(s) as arguments.
I'll fix the spacing between add_index and arguments.
There was a problem hiding this comment.
@krzysiek1507 done
There was a problem hiding this comment.
@arpitchauhan Yes, but we can put name into options hash (4th example).
If you check source, you will see that name will be consider too.
There was a problem hiding this comment.
@krzysiek1507 got it, thanks. Done.
arpitchauhan
force-pushed
the
bugfix/migration-name
branch
3 times, most recently
from
1e1f0ff
to
f5b1910
Compare
The migration AddMissingIndexes conflicts with a migration of similar name with other gems, eg acts_as_taggable_on 4.0.0. Its name has been made more specific in order to avoid such conflicts while installing migrations in a rails app. Also, since the migration will be copied over and run again, checks have been inserted ensuring the indexes being added don't exist already.
arpitchauhan
force-pushed
the
bugfix/migration-name
branch
from
f5b1910
to
a3d4e79
Compare
|
@krzysiek1507 is it possible for this PR to be included in 3.1.2? |
|
@damianlegawiec Similar MR for master branch was merged ( #7530 ). Can this be merged too? |
|
Sure @arpitchauhan and thanks for the fix! |
The migration AddMissingIndexes conflicts with a migration of similar
name in other gems, eg acts_as_taggable_on 4.0.0. Its name has been
made more specific in order to avoid such conflicts while installing
migrations in a rails app using