Skip to content

feat: save email contents in S3#463

Open
sakithb wants to merge 6 commits into
mainfrom
email-content-s3
Open

feat: save email contents in S3#463
sakithb wants to merge 6 commits into
mainfrom
email-content-s3

Conversation

@sakithb

@sakithb sakithb commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

fixes #447

@codecov-commenter

codecov-commenter commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.88136% with 32 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
worker/storage.go 58.06% 13 Missing ⚠️
worker/email_worker.go 56.52% 9 Missing and 1 partial ⚠️
backend/src/Service/Storage/FilesystemFactory.php 0.00% 5 Missing ⚠️
backend/src/Service/Send/SendContentStorage.php 87.50% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@sakithb sakithb marked this pull request as ready for review July 3, 2026 18:21
@supun-io supun-io changed the title Save email contents in storage service feat: save email contents in storage service Jul 6, 2026
@supun-io supun-io changed the title feat: save email contents in storage service feat: save email contents in S3 Jul 6, 2026

@supun-io supun-io left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • Update Env docs
  • Configure the default filesystem to be file, which saves uses local filesystem. For self-hosted setups, we will keep file the default. S3 would only be needed for advanced use cases. Use /app/media folder in the container
  • Deploy easy and prod compose files:
    • update compose.yaml to mount /app/media to a volume by default
  • Create a new docs page: Upgrade Guide. We will release this as 0.1.0, so, document that deployments need either update their compose.yaml to configure the volume, or setup s3
  • update prod deploy docs -> .env Configuration section to add S3_ variables after the OIDC_ variables. Mark it as option. Let the user know they can connect their own S3 platform if needed, and it's recommended for scaling.
  • fix go-staticcheck errors
  • codecov says some lines are not covered. Don't have to cover all, but make sure important ones are covered

return static function (ContainerConfigurator $containerConfigurator): void {
$containerConfigurator->parameters()
->set('env(HOSTING)', 'self') // Default to self-hosted
->set('app.filesystem_default', 's3')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

instead, following our convention, set defaults by setting them in .env file (applies to all envs).

$services->set(Filesystem::class)
->factory([FilesystemFactory::class, 'create'])
->args([
'%env(default:app.filesystem_default:string:FILESYSTEM)%',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just use default::string:FILESYSTEM

) {
}

public function store(string $uuid, SendContent $content): void

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

exception handling should be done: https://flysystem.thephpleague.com/docs/usage/exception-handling/

(important for s3, since it depends on network)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

document @throws here, and then catch it from where this is called

$this->filesystem->write($this->getJsonPath($uuid), $this->encodeJson($content));
}

public function getRaw(string $uuid): ?string

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

exception handling

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: save email contents outside of the database

3 participants