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

Fix native S3 config docs #24319

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

Conversation

hashhar
Copy link
Member

@hashhar hashhar commented Nov 30, 2024

Description

Current docs don't match the implementation and hence are incorrect and create confusion for user and make it more likely that users misconfigure things in the common usecases.

Additional context and related issues

Neither region nor endpoint are required. The defaults are auto-discovered by the AWS SDK.

The region might only need to be specified if running outside of EC2 or AWS SDK related settings/environment variables haven't been configured or if using a S3-compatible system where there is a single fixed region always.

The endpoint is also similarly usually not needed to be specified in real S3 unless using something like private endpoints. It's really only necessary when using a S3-compatible system.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

Neither region nor endpoint are required. The defaults are
auto-discovered by the AWS SDK.

The region might only need to be specified if running outside of EC2 or
AWS SDK related settings/environment variables haven't been configured
or if using a S3-compatible system where there is a single fixed region
always.

The endpoint is also similarly usually not needed to be specified in
real S3 unless using something like private endpoints. It's really only
necessary when using a S3-compatible system.
@hashhar hashhar requested a review from mosabua November 30, 2024 21:44
@cla-bot cla-bot bot added the cla-signed label Nov 30, 2024
@github-actions github-actions bot added the docs label Nov 30, 2024
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Lets just explain this better overall like you have it in the description. Ideally even with other S3-compatible system related info.

Also .. for both .. I assume the work as overrides.. we should document that.

@@ -25,9 +25,9 @@ support:
- Activate the native implementation for S3 storage support. Defaults to
`false`. Set to `true` to use S3 and enable all other properties.
* - `s3.endpoint`
- Required endpoint URL for S3.
- S3 service endpoint URL to communicate with.
Copy link
Member

Choose a reason for hiding this comment

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

Lets expand this a bit more as you explained in the description.

* - `s3.region`
- Required region name for S3.
- S3 region to communicate with.
Copy link
Member

Choose a reason for hiding this comment

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

Same as the other one..

@mosabua
Copy link
Member

mosabua commented Dec 2, 2024

Also fyi @electrum ... I think while regions is autofilled with the AWS SDK .. it is still a required field. For example .. even though it does not even exist as a concept in MinIO a value for region is still required .. although it is basically not used.

Copy link

This pull request has gone a while without any activity. Tagging for triage help: @mosabua

@github-actions github-actions bot added the stale label Dec 24, 2024
Copy link

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

@github-actions github-actions bot closed this Jan 14, 2025
@mosabua mosabua reopened this Jan 14, 2025
@mosabua
Copy link
Member

mosabua commented Jan 14, 2025

I still think this is useful and valid .. can you expand as requested @hashhar ?

@github-actions github-actions bot removed the stale label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants