`rake app:update` should update active_storage #33419

bogdanvlviv · 2018-07-23T00:52:17Z

Add foreign key to active_storage_attachments for blob_id via new migration

We need this in order to be able to add this migration for users that
use ActiveStorage during update their apps from Rails 5.2 to Rails 6.0.

Related to #33405

rake app:update should update active_storage

rake app:update should execute rake active_storage:update
if it is used in the app that is being updated.
It will add new active_storage's migrations to users' apps during update Rails.

Context #33405 (comment)

Also, see a related discussion in the Campfire:
https://3.basecamp.com/3076981/buckets/24956/chats/12416418@1236713081

rails-bot · 2018-07-23T00:52:19Z

r? @kamipo

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

...grate/20180723000244_add_foreign_key_constraint_to_active_storage_attachments_for_blob_id.rb

georgeclaghorn · 2018-07-23T12:37:44Z

I don’t think we want to copy two migrations into new Rails apps (or existing Rails apps where Active Storage is newly installed). Ideally, we’d copy one migration into new Active Storage installations and add the foreign key migration when an existing installation is upgraded.

kaspth · 2018-07-28T12:18:36Z

Yep, we have to make it work like @georgeclaghorn suggests. Perhaps we should add a hidden active_storage:update task that rails app:update could use? Perhaps we could check whether the initial migration is in db/migrate to decide which migration to copy (this might get tricky for multiple db setups).

Finally, the upgrade migration should be removed from the codebase in Rails 6.1.

bogdanvlviv · 2018-08-07T21:51:23Z

I've been thinking about that. We could go with this approach, but it looks really complicated to me and I don't see any benefits from doing that since as I understand it only prevents adding one more migration to a newly generated app(but I don't think that it is an issue since it is how a rails engine works).

Perhaps we could check whether the initial migration is in db/migrate to decide which migration to copy (this might get tricky for multiple db setups).

Currently, I have one concern about that. Example:
Imagine we created a new app(with ActiveStorage) on Rails 6.0.
Executed CreateActiveStorageTables migration that is already with https://github.com/rails/rails/blob/master/activestorage/db/migrate/20170806125915_create_active_storage_tables.rb#L23
How should we know whether copy the migration on execution of rails app:update since these changes have already applied by CreateActiveStorageTables?

Also, I'm not sure that it is a good idea to update existing migrations, otherwise we should merge #32042 I guess.

Finally, the upgrade migration should be removed from the codebase in Rails 6.1.`

Currently, I lean to the engine's approach with multiple migrations. It would be easier for us to maintain and execution of ActiveStorage's tests would test the migration in this way, see #33419 (comment).

Let's discuss about approach we want to go with.

bogdanvlviv · 2018-08-09T10:57:33Z

Since 945e2c4 was backported to 5-2-stable, we should implement the upgrading approach for 5.2.2 too.

kaspth · 2018-08-12T08:33:40Z

Since 945e2c4 was backported to 5-2-stable, we should implement the upgrading approach for 5.2.2 too.

Let's just catch them on 6.0 and get the upgrading behavior right. rails app:update is not intended for point releases.

kaspth · 2018-08-12T08:37:51Z

@bogdanvlviv I still think trying to prevent the extra migration when we don't have to is a win for end users. New users just get the one migration they need, while upgrading users are still taken care of.

How should we know whether copy the migration on execution of rails app:update since these changes have already applied by CreateActiveStorageTables?

Grep the file for the foreign_key constraint? I agree that it isn't the prettiest. For now I think we're better suited finding something that makes this work, however ugly. And then see if we discover a better way doing that. Or if it's too ugly, we can cut it and do 2 migrations.

bogdanvlviv · 2018-08-12T12:46:15Z

Let's just catch them on 6.0 and get the upgrading behavior right. rails app:update is not intended for point releases.

@kaspth I think we should consider reverting 945e2c4 and relative changes in 5-2-stable since they looks really significant as for next 5.2.2 tiny release?
/cc @georgeclaghorn

matthewd · 2018-08-12T13:21:22Z

Agree.. that doesn't look suitable for a patch release to me.

bogdanvlviv · 2018-09-16T18:00:46Z

activestorage/lib/tasks/activestorage.rake

@@ -12,4 +12,15 @@ namespace :active_storage do
      Rake::Task["app:active_storage:install:migrations"].invoke
    end
  end
+
+  # desc "Copy over the migrations needed to the application upgrading"
+  task update: :environment do


Please let me know what do you think about this approach?

Since we decided to edit the existing migration "create_active_storage_tables" in #33405 in order to generate only one migration for a new app I think we should consider merging this PR - #32042 (but I'm not sure whether it makes sense for now)

/cc @georgeclaghorn, @matthewd

bogdanvlviv · 2018-09-24T21:19:04Z

Could you please attach the milestone 6.0.0 to this PR (Unfortunately, I have no permission to do that). I don't want to lose this PR among other PRs since it should be done before we release Rails 6.0.

bogdanvlviv · 2018-12-04T22:32:01Z

Please let me know if there anything I should do?
Also, there is the question I would like to discuss - #33419 (comment)

…igration We need this in order to be able to add this migration for users that use ActiveStorage during update their apps from Rails 5.2 to Rails 6.0. Related to rails#33405 `rake app:update` should update active_storage `rake app:update` should execute `rake active_storage:update` if it is used in the app that is being updated. It will add new active_storage's migrations to users' apps during update Rails. Context rails#33405 (comment) Also, see a related discussion in the Campfire: https://3.basecamp.com/3076981/buckets/24956/chats/12416418@1236713081

georgeclaghorn · 2019-01-16T22:37:56Z

Sorry for the delay in reviewing this. This looks good. 👍

Could you please move the update migration to activestorage/db/migrate/update? (I tested this locally and it works.)

bogdanvlviv · 2019-01-16T22:52:03Z

Could you please move the update migration to activestorage/db/migrate/update? (I tested this locally and it works.)

Not sure we can do that since rails active_storage:install will add this migration in that case.

blog$ rails active_storage:install
Copied migration 20190116224746_create_active_storage_tables.active_storage.rb from active_storage
Copied migration 20190116224747_add_foreign_key_constraint_to_active_storage_attachments_for_blob_id.active_storage.rb from active_storage

georgeclaghorn · 2019-01-16T23:02:39Z

Ugh, yes. I tested installing Active Storage in a 5.2 app and upgrading to 6.0 but not installing Active Storage in a 6.0 app. 🤦‍♂️

bogdanvlviv · 2019-01-16T23:05:21Z

Thank you for the review! ❤️

…igration In rails#33419, we added this migration to properly upgrade Active Storage from 5.2 to 6.0 On Rails 6.1 `rails app:update` shouldn't add this migration to users' app. Note that, I've left implementation that makes `rails app:update` to generate migrations for users' app that are in `activestorage/db/update_migrate/` because we are likely to need it e.g.: rails#34935, rails#36835.

…igration In rails/rails#33419, we added this migration to properly upgrade Active Storage from 5.2 to 6.0 On Rails 6.1 `rails app:update` shouldn't add this migration to users' app. Note that, I've left implementation that makes `rails app:update` to generate migrations for users' app that are in `activestorage/db/update_migrate/` because we are likely to need it e.g.: rails/rails#34935, rails/rails#36835.

rails-bot assigned kamipo Jul 23, 2018

bogdanvlviv commented Jul 23, 2018

View reviewed changes

...grate/20180723000244_add_foreign_key_constraint_to_active_storage_attachments_for_blob_id.rb Outdated Show resolved Hide resolved

bogdanvlviv force-pushed the update-active_storage branch 2 times, most recently from 39434be to 36ea540 Compare September 16, 2018 17:53

bogdanvlviv commented Sep 16, 2018

View reviewed changes

bogdanvlviv force-pushed the update-active_storage branch from 36ea540 to 1e28b88 Compare September 20, 2018 20:20

georgeclaghorn added this to the 6.0.0 milestone Sep 24, 2018

gmcgibbon added railties activestorage labels Sep 28, 2018

bogdanvlviv force-pushed the update-active_storage branch from 1e28b88 to 23b69d8 Compare January 4, 2019 19:40

rails-bot bot added the activerecord label Jan 4, 2019

bogdanvlviv force-pushed the update-active_storage branch 2 times, most recently from de9032a to 3810759 Compare January 8, 2019 18:25

bogdanvlviv force-pushed the update-active_storage branch from 3810759 to 2bad3f4 Compare January 16, 2019 13:13

georgeclaghorn merged commit 9e34df0 into rails:master Jan 16, 2019

bogdanvlviv deleted the update-active_storage branch January 16, 2019 23:05

bogdanvlviv mentioned this pull request Jan 17, 2019

Allow configure services for individual attachments #34935

Merged

bogdanvlviv mentioned this pull request Aug 12, 2019

Remove AddForeignKeyConstraintToActiveStorageAttachmentsForBlobId migration #36915

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`rake app:update` should update active_storage #33419

`rake app:update` should update active_storage #33419

bogdanvlviv commented Jul 23, 2018 •

edited

rails-bot commented Jul 23, 2018

georgeclaghorn commented Jul 23, 2018

kaspth commented Jul 28, 2018

bogdanvlviv commented Aug 7, 2018 •

edited

bogdanvlviv commented Aug 9, 2018 •

edited

kaspth commented Aug 12, 2018

kaspth commented Aug 12, 2018

bogdanvlviv commented Aug 12, 2018

matthewd commented Aug 12, 2018

bogdanvlviv Sep 16, 2018 •

edited

bogdanvlviv commented Sep 24, 2018

bogdanvlviv commented Dec 4, 2018

georgeclaghorn commented Jan 16, 2019

bogdanvlviv commented Jan 16, 2019

georgeclaghorn commented Jan 16, 2019 •

edited

bogdanvlviv commented Jan 16, 2019

rake app:update should update active_storage #33419

rake app:update should update active_storage #33419

Conversation

bogdanvlviv commented Jul 23, 2018 • edited

rails-bot commented Jul 23, 2018

georgeclaghorn commented Jul 23, 2018

kaspth commented Jul 28, 2018

bogdanvlviv commented Aug 7, 2018 • edited

bogdanvlviv commented Aug 9, 2018 • edited

kaspth commented Aug 12, 2018

kaspth commented Aug 12, 2018

bogdanvlviv commented Aug 12, 2018

matthewd commented Aug 12, 2018

bogdanvlviv Sep 16, 2018 • edited

Choose a reason for hiding this comment

bogdanvlviv commented Sep 24, 2018

bogdanvlviv commented Dec 4, 2018

georgeclaghorn commented Jan 16, 2019

bogdanvlviv commented Jan 16, 2019

georgeclaghorn commented Jan 16, 2019 • edited

bogdanvlviv commented Jan 16, 2019

`rake app:update` should update active_storage #33419

`rake app:update` should update active_storage #33419

bogdanvlviv commented Jul 23, 2018 •

edited

bogdanvlviv commented Aug 7, 2018 •

edited

bogdanvlviv commented Aug 9, 2018 •

edited

bogdanvlviv Sep 16, 2018 •

edited

georgeclaghorn commented Jan 16, 2019 •

edited