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

[#6531] Store raw emails with ActiveStorage #6539

Merged
merged 9 commits into from
Dec 20, 2021

Conversation

gbp
Copy link
Member

@gbp gbp commented Sep 29, 2021

Relevant issue(s)

Fixes #6530
Fixes #6531

What does this do?

  • Adds ActiveStorage support for raw emails
  • Add rake task to migrate raw emails on disk to a ActiveStorage backed file store.

Why was this needed?

ActiveStorage is integrated with cloud based services to so file stores can exist on S3. This will help installations as on disk raw emails file stores can get quite large. This makes migrated servers difficult and limits the ability to run more than one application instance.

Implementation notes

The application doesn't require migration straight away and will fall back to the old file store if the files aren't attached.

@gbp gbp force-pushed the 6531-store-raw-emails-with-activestorage branch 2 times, most recently from 4e79c16 to 55efdc3 Compare October 1, 2021 11:17
@gbp gbp force-pushed the 6531-store-raw-emails-with-activestorage branch 5 times, most recently from 45c6213 to 430f2b9 Compare November 26, 2021 15:19
@gbp gbp marked this pull request as ready for review November 26, 2021 15:54
@gbp gbp requested a review from garethrees November 26, 2021 15:54
@garethrees garethrees self-assigned this Nov 26, 2021
Copy link
Member

@garethrees garethrees left a comment

Choose a reason for hiding this comment

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

Magic! 🧙 Couple of minor tweaks to address but all good in principle. Haven't run it with S3 backend but – I presume – one wouldn't really notice much change!

Probably want to coordinate when we merge this to make sure we've got the config files in place and a plan for migration.

app/models/raw_email.rb Outdated Show resolved Hide resolved
app/models/raw_email.rb Outdated Show resolved Hide resolved
spec/models/raw_email_spec.rb Show resolved Hide resolved
lib/tasks/storage.rake Outdated Show resolved Hide resolved
config/storage.yml-example Show resolved Hide resolved
config/storage.yml-example Show resolved Hide resolved
app/models/raw_email.rb Outdated Show resolved Hide resolved
lib/configuration.rb Show resolved Hide resolved
doc/CHANGES.md Outdated Show resolved Hide resolved
app/models/raw_email.rb Outdated Show resolved Hide resolved
@gbp gbp requested a review from garethrees December 1, 2021 13:48
@gbp
Copy link
Member Author

gbp commented Dec 1, 2021

@garethrees any other thoughts on the fix ups before I merge?

Copy link
Member

@garethrees garethrees left a comment

Choose a reason for hiding this comment

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

any other thoughts on the fix ups before I merge?

Looks great! Nothing significant that I can think of, but a couple of small inline comments. Don't need to re-review.

doc/CHANGES.md Outdated Show resolved Hide resolved
lib/configuration.rb Outdated Show resolved Hide resolved
lib/tasks/storage.rake Show resolved Hide resolved
@gbp gbp force-pushed the 6531-store-raw-emails-with-activestorage branch from 48fedd5 to 8153c03 Compare December 2, 2021 07:05
@gbp gbp mentioned this pull request Dec 2, 2021
@gbp gbp force-pushed the 6531-store-raw-emails-with-activestorage branch from 8153c03 to 1f16801 Compare December 6, 2021 12:50
@gbp gbp force-pushed the 6531-store-raw-emails-with-activestorage branch from 8b20931 to 6a32693 Compare December 7, 2021 17:11
@gbp gbp requested a review from garethrees December 10, 2021 09:55
Copy link
Member

@garethrees garethrees left a comment

Choose a reason for hiding this comment

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

This all looks great! Such an improvement!

The performance is noticeably slower, obviously, and that might get passed on to users since it is possible for us to do a first-parse of a raw email during the request cycle. I expect many will be parsed before storage:promote happens, but they might not be. We did once proactively parse pro requests, so we could re-introduce something like that if it becomes a problem. I think we'll want to keep an eye on the amount of GET requests we're seeing, since the only other place we'd see a GET, I think, is via /admin/raw_emails/:id.format.

lib/tasks/storage/storage.rb Outdated Show resolved Hide resolved
Defaults to disk storage but adds example config and the gems required
to store in Amazon S3 (or S3 like services), Azure or Google Cloud
Storage.

Fixes #6530
Preparing for a new context when we move to a `ActiveStorage` backed
file store.
When the `ActiveStorage::Blob` file isn't attached then use the old file
store.

There are no specs currently for the `#directory` and `#filepath`
methods. This due to a `if Rails.env.test?` conditional in the model.
These methods won't be used when a install switches to `ActiveStorage`
and so they can be deprecated.
This won't be used in 0.42 as in 0.41 installs will be required to
migrate to `ActiveStorage`.
Add tasks to:
1. mirror files from primary ActiveStorage backend to secondary backends
2. promote mirrored files to be served from the secondary backend
3. unlink primary file if mirrored and promoted to secondary backend.
@gbp gbp force-pushed the 6531-store-raw-emails-with-activestorage branch from 6a32693 to f70328e Compare December 16, 2021 11:55
@gbp gbp merged commit 26f6c21 into develop Dec 20, 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.

Store RawEmail#data in Active Storage Install and configure Active Storage
2 participants