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

Add companion option for S3 ServerSideEncryption and update docs with missing options #5348

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kengoldfarb
Copy link

No description provided.

Copy link
Contributor

Diff output files
diff --git a/packages/@uppy/companion/lib/server/controllers/s3.js b/packages/@uppy/companion/lib/server/controllers/s3.js
index 7526f30..f108410 100644
--- a/packages/@uppy/companion/lib/server/controllers/s3.js
+++ b/packages/@uppy/companion/lib/server/controllers/s3.js
@@ -120,6 +120,9 @@ module.exports = function s3(config) {
     if (config.acl != null) {
       params.ACL = config.acl;
     }
+    if (config.serverSideEncryption != null) {
+      params.ServerSideEncryption = config.serverSideEncryption;
+    }
     client.send(new CreateMultipartUploadCommand(params)).then((data) => {
       res.json({
         key: data.Key,

@@ -563,6 +563,17 @@ app.use(
When signing on the client, this function will only be called for multipart
uploads.

##### `s3.acl`
Copy link
Member

Choose a reason for hiding this comment

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

The companion docs put the env var and JS option in the same header. This option already exists: https://uppy.io/docs/companion/#companion_aws_acl so you should add the JS var in front of it (see other options).

Or maybe the env var should be moved here?

@Murderlon Murderlon requested a review from mifi July 22, 2024 07:31
##### `s3.serverSideEncryption`

Specify the
[Server-Side Encryption](https://docs.aws.amazon.com/AmazonS3/latest/dev/serv-side-encryption.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we maybe also link to https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-aws-sdk-client-s3/Variable/ServerSideEncryption/ - so the user knows which values are valid?

also I think we need a corresponding env variable. maybe COMPANION_AWS_S3_SERVER_SIDE_ENCRYPTION

@Murderlon
Copy link
Member

ping @kengoldfarb

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.

3 participants