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.

Sign up for GitHub

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

Jump to bottom

Active storage migrations service #7902

Merged
merged 20 commits into from Jul 23, 2021
Merged

Active storage migrations service #7902

merged 20 commits into from Jul 23, 2021

Conversation

entantoencuanto
Copy link
Contributor

@entantoencuanto entantoencuanto commented Apr 27, 2021 •
edited

🎩 What? Why?

This PR add the tasks required to migrate files in CarrierWave to ActiveStorage attachments:

  • Adds a migration task which export results to a log checking also if the origin and destination files are identical and generates a csv with the mappings of paths of all migrated attachments in tmp/attachment_mappings.csv. If the task detects that there already exists an attached file with ActiveStorage the export is skipped and no checks are made. To run this task execute:
rails decidim:active_storage_migrations:migrate_from_carrierwave_to_active_storage
  • Adds a check task which ensures that all migrated files are identical. This is made in the first task when a migration is performed, but skipped for already existing attachments. To run this task execute:
rails decidim:active_storage_migrations:check_migration_from_carrierwave_to_active_storage

Both tasks generate a log file in log/ with the result of the migration of each file and/or the checksum verification result.

📌 Related Issues

Link your PR to an issue

Testing

Describe the best way to test or validate your PR.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@entantoencuanto
Copy link
Contributor Author

I've change the seeds to generate much more attachments and seed attachments for all uploaders defined with CW. This modified seed process creates 13322 attachments with a total size of 584M.

  • First I made the test using as storage the local filesystem, both for CW and AS and the execution took 50 minutes of which 14 were to export the paths mapping.
  • Then I made the same test using a S3 bucket both for CW an AS ant the execution took about 8:30h

In both cases all the files were migrated with the same checksum. I've used the files used by seeds usually and added another one for some new attachments. These are the files:

  • homepage_image.jpg - 1.6M
  • city.jpeg - 105K
  • city2.jpeg 127K
  • logo.png - 3.9K
  • Exampledocument.pdf - 17K
  • avatar.png - 323K

It would be nice to test the task in a more realistic environment

@entantoencuanto entantoencuanto mentioned this pull request Apr 30, 2021
Closed
@entantoencuanto entantoencuanto force-pushed the active_storage_migrations_service branch from 5f6b12d to 20a2059 Compare April 30, 2021 10:59
@entantoencuanto entantoencuanto force-pushed the active_storage_migrations_service branch 2 times, most recently from 4544f40 to 7346e1b Compare June 29, 2021 15:39
@entantoencuanto
Copy link
Contributor Author

Rebased on top of active_storage_migration of #7598

@leio10 leio10 force-pushed the active_storage_migrations_service branch from 7346e1b to c65d370 Compare July 15, 2021 13:39
@leio10 leio10 changed the base branch from develop to active_storage_migration July 15, 2021 13:40
Base automatically changed from active_storage_migration to develop July 20, 2021 11:13
entantoencuanto and others added 13 commits July 20, 2021 13:49
@entantoencuanto
Add service to migrate CarrierWave attachments to ActiveStorage
ef4f4fb
@ferblape @entantoencuanto
WIP
d4166d0
@entantoencuanto
Change migration to add checks, migrate content blocks and use CW upl…
d58f10e 
…oaders
@entantoencuanto
Include check info in migration tasks
1f1e085
@entantoencuanto
Remove duplication
15264e7
@entantoencuanto
Prepare migration service to work with CarrierWave::Storage::Fog atta…
ef02d89 
…chments
@entantoencuanto
Disable rubocop offense to allow excess of parameters in migrate_atta…
60f2ca5 
…chment! CarrierWaveMigratorService method
@entantoencuanto
Change checksum calculation of file depending of its type
266d2ed 
When attachment is remote and large enough the file is stored in a
Tempfile, but if it's small URI.open returns a StringIO. This methods
calculates the checksum correctly both when original attachment is local
or stored remotely
@entantoencuanto
Fix rubocop offense
ddebc4f
@entantoencuanto
Remove unused method
6db96b0
@leio10 @entantoencuanto
fix: don't migrate models from modules not loaded
9595457
@entantoencuanto
Fix file cw_file method when cw attachments are stored in local files…
c69a2d7 
…ystem
@entantoencuanto
Fix lint issues
088c28c
@entantoencuanto entantoencuanto force-pushed the active_storage_migrations_service branch from 9405466 to 088c28c Compare July 20, 2021 13:28
@ahukkanen ahukkanen mentioned this pull request Jul 21, 2021
Closed
@entantoencuanto entantoencuanto marked this pull request as ready for review July 21, 2021 15:12
leio10
leio10 approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@leio10 leio10 left a comment

Choose a reason for hiding this comment

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

Awesome work!! 👏👏

@leio10 leio10 merged commit 9ecc7f3 into develop Jul 23, 2021
@leio10 leio10 deleted the active_storage_migrations_service branch July 23, 2021 13:47
entantoencuanto added a commit that referenced this pull request Jul 26, 2021
@entantoencuanto
Merge branch 'develop' into 7714-quilljs
e488132 
* develop: (32 commits)
  Remove obsolete rake webpack task (#8237)
  Active storage migrations service (#7902)
  Fix content type delegation to blank attachments (#8230)
  Evote bug fixing (#8220)
  Fix the proposal data migration for proposals without authors or organization (#8015)
  Bump addressable version because security issues (#8229)
  Online meetings iframe visibility with time (#8097)
  Meetings iframe and iframe URL (#8096)
  Remove flaky test on meetings (#8226)
  Fix broken tests after problematic PRs (#8224)
  Apply permissions system to comments (#8035)
  Set current_component as commentable when commentable is a participatory space (#8189)
  Fix don't require inactive authorization handlers (#8122)
  Improve metrics calculations performance (#8215)
  Fix performance issue in notification settings page (#8155)
  Active storage migration (#7598)
  Update manual installation guide in documentation (#8217)
  Load JS configuration in elections focus mode layout (#8213)
  Fix user activity pagination when there are hidden items (#8202)
  Make it possible to define SCSS settings overrides from modules (#8198)
  ...
roxanaopr pushed a commit to tremend-cofe/decidim that referenced this pull request Jul 29, 2021
@entantoencuanto @ferblape
@leio10 @roxanaopr
Active storage migrations service (decidim#7902)
99d9d00 
* Add service to migrate CarrierWave attachments to ActiveStorage

* WIP

* Change migration to add checks, migrate content blocks and use CW uploaders

* Include check info in migration tasks

* Remove duplication

* Prepare migration service to work with CarrierWave::Storage::Fog attachments

* Disable rubocop offense to allow excess of parameters in migrate_attachment! CarrierWaveMigratorService method

* Change checksum calculation of file depending of its type

When attachment is remote and large enough the file is stored in a
Tempfile, but if it's small URI.open returns a StringIO. This methods
calculates the checksum correctly both when original attachment is local
or stored remotely

* Fix rubocop offense

* Remove unused method

* fix: don't migrate models from modules not loaded

* Fix file cw_file method when cw attachments are stored in local filesystem

* Fix lint issues

* Force the loading of the models in MIGRATION_ATTRIBUTES constant

* Skip allowlists in CW uploaders

* Use relative path in favicon_link_tag

* Update CHANGELOG.md

* Use protocol if required in urls generated by application_uploader

* Revert "Use relative path in favicon_link_tag"

This reverts commit 09542dd.

Co-authored-by: Fernando Blat <fernando@blat.es>
Co-authored-by: Leonardo Diez <leiodd@gmail.com>
roxanaopr pushed a commit to tremend-cofe/decidim that referenced this pull request Aug 10, 2021
@entantoencuanto @ferblape
@leio10 @roxanaopr
Active storage migrations service (decidim#7902)
d6e6dab 
* Add service to migrate CarrierWave attachments to ActiveStorage

* WIP

* Change migration to add checks, migrate content blocks and use CW uploaders

* Include check info in migration tasks

* Remove duplication

* Prepare migration service to work with CarrierWave::Storage::Fog attachments

* Disable rubocop offense to allow excess of parameters in migrate_attachment! CarrierWaveMigratorService method

* Change checksum calculation of file depending of its type

When attachment is remote and large enough the file is stored in a
Tempfile, but if it's small URI.open returns a StringIO. This methods
calculates the checksum correctly both when original attachment is local
or stored remotely

* Fix rubocop offense

* Remove unused method

* fix: don't migrate models from modules not loaded

* Fix file cw_file method when cw attachments are stored in local filesystem

* Fix lint issues

* Force the loading of the models in MIGRATION_ATTRIBUTES constant

* Skip allowlists in CW uploaders

* Use relative path in favicon_link_tag

* Update CHANGELOG.md

* Use protocol if required in urls generated by application_uploader

* Revert "Use relative path in favicon_link_tag"

This reverts commit 09542dd.

Co-authored-by: Fernando Blat <fernando@blat.es>
Co-authored-by: Leonardo Diez <leiodd@gmail.com>
takahashim added a commit to takahashim/decidim-cfj that referenced this pull request Jun 13, 2022
@takahashim
Add Decidim::Cw::ApplicationUploader instead of Decidim::ApplicationU…
a3a6481 
…ploader

and add `url` method that was originally added in Decidim::ApplicationUploader

see also: decidim/decidim#7902
@takahashim takahashim mentioned this pull request Jun 13, 2022
Closed
6 tasks
@ahukkanen ahukkanen mentioned this pull request Aug 1, 2022
Closed
@alecslupu alecslupu mentioned this pull request Dec 4, 2023
Closed
@andreslucena andreslucena mentioned this pull request Dec 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leio10 leio10 leio10 approved these changes

Assignees
No one assigned
Labels
in-review module: core target: developer-experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@entantoencuanto @leio10 @andreslucena @ferblape