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 4 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
README.md Outdated
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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The provided config sets them to '' by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that because I was asked to back in September. I have now updated the docs to reflect that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what the consequences are for any kind of S3 flavour if this is set to an empty string. Have you tested this somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jessebot I still have some doubts if an empty string overrides Nextcloud's defaults.

@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
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
…Limit, version, and verify_bucket_exists

Signed-off-by: jessebot <jessebot@linux.com>
Signed-off-by: Jesse Hitch <jessebot@linux.com>
Signed-off-by: Jesse Hitch <jessebot@linux.com>
Signed-off-by: Jesse Hitch <jessebot@linux.com>
@jessebot jessebot force-pushed the add-missing-s3-variables branch from d7baf2f to 1581991 Compare January 2, 2025 16:19
@jessebot
Copy link
Contributor Author

jessebot commented Jan 5, 2025

@joshtrichards or @J0WI could you take another look at this when you have a moment?

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
3 participants