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

Make ActiveStorage update migration compatible with install migration #41193

Conversation

etiennebarrie
Copy link
Contributor

If you install ActiveStorage in rails 6.1, then use rails app:update, the service name migration handles that case but not the variant records one, which fails trying to re-create the table.

Summary

I updated an app to Rails 6.1, but forgot to run rails app:update immediately. Then later I installed ActiveStorage, and finally came back to run the update task.
When I saw the update migrations being created in the app, I assumed they would fail but was pleasantly surprised that the service name passed by checking for the presence of the column. So I felt maybe we'd want to do the same to the other migration. We can also assume users will understand and just remove the migrations.

Repro steps:

$ rails _6.1.1_ new my_app && cd my_app
$ rails active_storage:install && rails db:migrate
$ rails app:update && rails db:migrate
# fails on CreateActiveStorageVariantRecords

Other Information

It works perfectly in the regular case:

$ rails _6.0.3.4_ new my_app && cd my_app
$ rails active_storage:install && rails db:migrate
$ $EDITOR Gemfile && bundle update # update to 6.1
$ yes | rails app:update && rails db:migrate

def change
def up
return if table_exists?(:active_storage_variant_records)

create_table :active_storage_variant_records do |t|
Copy link
Member

Choose a reason for hiding this comment

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

Let's change to

Suggested change
create_table :active_storage_variant_records do |t|
create_table :active_storage_variant_records, if_not_exists: true do |t|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right I had forgotten about this 👍
Also works with as a reversible migration directly.

@etiennebarrie etiennebarrie force-pushed the activestorage-compatible-update-migration branch from d727de1 to 11a4ccd Compare January 22, 2021 04:15
@rails-bot
Copy link

rails-bot bot commented Apr 23, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Apr 23, 2021
@etiennebarrie
Copy link
Contributor Author

This is still good to go!

@rails-bot rails-bot bot removed the stale label Apr 23, 2021
If you install active_storage in rails 6.1, then use rails app:update,
the service name migration handles that case but not the variant records
one, which fails trying to re-create the table.

Co-authored-by: Adrianna Chang <adrianna.chang@shopify.com>
@etiennebarrie etiennebarrie force-pushed the activestorage-compatible-update-migration branch from 11a4ccd to 51432c6 Compare June 29, 2021 14:21
@rails-bot
Copy link

rails-bot bot commented Sep 27, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Sep 27, 2021
@ghiculescu ghiculescu removed the stale label Sep 27, 2021
@rafaelfranca rafaelfranca merged commit 479583a into rails:main Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants