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

allow setting s3 concurrency, proxy, timeout, uploadPartSize, putSizeLimit, version, and verify_bucket_exists #2271

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jessebot
Copy link
Contributor

@jessebot jessebot commented Jul 26, 2024

Adds missing S3 related environment variables. Fixes #2270

Per the docs here's the missing supported S3 parameters:

  • concurrency defaults to 5 [Note: This defines the maximum number of concurrent multipart uploads]

  • proxy defaults to false

  • timeout defaults to 15

  • uploadPartSize defaults to 524288000

  • putSizeLimit defaults to 104857600

  • version defaults to latest

  • verify_bucket_exists defaults to true [Note: Setting this to false after confirming the bucket has been created may provide a performance benefit, but may not be possible in multibucket scenarios.]

This also helps us get ready to merge nextcloud/helm#614 downstream 🙏

Let me know if you need anything else :)

.config/s3.config.php Outdated Show resolved Hide resolved
Comment on lines +195 to +285
- `OBJECTSTORE_S3_CONCURRENCY` (default: `5`) defines the maximum number of concurrent multipart uploads
- `OBJECTSTORE_S3_PROXY` (default: `false`)
- `OBJECTSTORE_S3_TIMEOUT` (default: `15`)
- `OBJECTSTORE_S3_UPLOADPARTSIZE` (default: `524288000`)
- `OBJECTSTORE_S3_PUTSIZELIMIT` (default: `104857600`)
- `OBJECTSTORE_S3_VERSION` (default: `latest`)
- `OBJECTSTORE_S3_VERIFY_BUCKET_EXISTS` (default: `true`)
Copy link
Contributor Author

@jessebot jessebot Sep 20, 2024

Choose a reason for hiding this comment

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

do we also want to change these to "not set by default"? I can also understand keeping the defaults here, just so people don't have to cross reference with the nextcloud/server docs. Up to you, happy to accommodate either :)

@jessebot
Copy link
Contributor Author

@joshtrichards wanted to gently follow up and ask if anything else is needed here?

@jessebot
Copy link
Contributor Author

@J0WI or @joshtrichards can I ask if there's anything else needed here to get this merged into the default branch? Sorry to be a pest. I just want to make sure this gets merged so we can support it in the helm chart properly.

@jessebot jessebot force-pushed the add-missing-s3-variables branch from 10d6edd to 2b545ea Compare December 17, 2024 18:13
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.

add additional S3 related environment variables
2 participants