Skip to content

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.

@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
mosabua
mosabua previously requested changes Dec 1, 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.

`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 service endpoint URL to communicate with.
* - `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.

@github-actions
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
@github-actions
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
@github-actions
Copy link

github-actions bot commented Feb 6, 2025

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

@github-actions github-actions bot added the stale label Feb 6, 2025
@mosabua
Copy link
Member

mosabua commented Feb 6, 2025

Ping @hashhar

@github-actions github-actions bot removed the stale label Feb 7, 2025
@github-actions
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 Feb 28, 2025
@github-actions
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 Mar 21, 2025
@mosabua mosabua reopened this Mar 21, 2025
@mosabua
Copy link
Member

mosabua commented Mar 21, 2025

We should get this in after expanding .. let me know if you need help @hashhar

@mosabua mosabua added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Mar 21, 2025
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.
@ebyhr ebyhr force-pushed the hashhar/fix-s3-required-config branch from 380a990 to c3ed557 Compare September 15, 2025 23:39
@ebyhr ebyhr merged commit 17e7b42 into trinodb:master Sep 15, 2025
3 of 8 checks passed
@github-actions github-actions bot added this to the 477 milestone Sep 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed docs stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed.

Development

Successfully merging this pull request may close these issues.

4 participants