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.

Sign up for GitHub

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

Jump to bottom

Add support for ACL config in native S3 file system #21176

Merged
merged 2 commits into from
Mar 26, 2024
Merged

Add support for ACL config in native S3 file system #21176

merged 2 commits into from
Mar 26, 2024

Conversation

anusudarsan
Copy link
Member

@anusudarsan anusudarsan commented Mar 20, 2024 •
edited
Loading

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text:

# Section
* Add support for acl config in native s3 filesystem

@cla-bot cla-bot bot added the cla-signed label Mar 20, 2024
@anusudarsan anusudarsan force-pushed the anu/more-native-s3-options branch 2 times, most recently from e8de3f5 to e7b9117 Compare March 21, 2024 13:58
electrum
electrum reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

Let's remove the custom signer commit. We don't want to load arbitrary classes, plus there is the problem of how a user would actually use the feature and provide configuration. Is there a specific use case for this?

@anusudarsan anusudarsan force-pushed the anu/more-native-s3-options branch from e7b9117 to 09a07e7 Compare March 22, 2024 16:41
@anusudarsan anusudarsan changed the title Add custom signer and acl type configs to native s3 filesystem Add support for acl config in native s3 filesystem Mar 22, 2024
@anusudarsan
Copy link
Member Author

anusudarsan commented Mar 22, 2024 •
edited
Loading

Let's remove the custom signer commit. We don't want to load arbitrary classes, plus there is the problem of how a user would actually use the feature and provide configuration. Is there a specific use case for this?

I removed it from this PR. Im yet to hear on the actual use-case for custom signer.

@anusudarsan anusudarsan force-pushed the anu/more-native-s3-options branch 2 times, most recently from 648840a to 425ac97 Compare March 22, 2024 17:39
@anusudarsan
Copy link
Member Author

@findepi or @electrum can one of you trigger tests with secrets?

@electrum
Copy link
Member

/test-with-secrets sha=425ac97974b1f88bfbc2a3e8d9e3aafd5d9bef7e

@github-actions GitHub Actions
Copy link

The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/8395787086

@anusudarsan anusudarsan force-pushed the anu/more-native-s3-options branch from 425ac97 to 95f1e3a Compare March 25, 2024 18:05
@github-actions github-actions bot added the docs label Mar 25, 2024
@anusudarsan anusudarsan force-pushed the anu/more-native-s3-options branch from 95f1e3a to 3ed1e15 Compare March 25, 2024 18:16
electrum
electrum reviewed Mar 25, 2024
View reviewed changes
lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemConfig.java Outdated
}

@Config("s3.canned-acl")
@ConfigDescription("Canned ACL (predefined grants) to manage access to buckets and objects")
Copy link
Member

Choose a reason for hiding this comment

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

Remove "buckets" since in this context it's only relevant for objects (canned ACLs can also be set at the bucket level but we're not doing that here)

Canned ACL (predefined grants) to manage access to objects

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, looks like this one was not updated

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry . missed it. updated now

@anusudarsan anusudarsan force-pushed the anu/more-native-s3-options branch from 3ed1e15 to 0a6825e Compare March 25, 2024 20:07
@electrum
Copy link
Member

/test-with-secrets sha=0a6825ecec58fea7b6fce8fb8a1218579b756bcd

@github-actions GitHub Actions
Copy link

The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/8427028437

@findepi findepi mentioned this pull request Mar 25, 2024
Closed
@findepi findepi changed the title Add support for acl config in native s3 filesystem Add support for ACL config in native S3 filesystem Mar 25, 2024
@anusudarsan anusudarsan force-pushed the anu/more-native-s3-options branch from 0a6825e to 0d2ba4d Compare March 26, 2024 17:32
@electrum electrum merged commit 1736da9 into trinodb:master Mar 26, 2024
53 of 61 checks passed
@github-actions github-actions bot added this to the 444 milestone Mar 26, 2024
@anusudarsan anusudarsan deleted the anu/more-native-s3-options branch March 26, 2024 18:28
@mosabua mosabua changed the title Add support for ACL config in native S3 filesystem Add support for ACL config in native S3 file system Mar 26, 2024
@mosabua mosabua mentioned this pull request Mar 26, 2024
Merged
@colebow colebow mentioned this pull request Mar 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@electrum electrum electrum approved these changes

@findinpath findinpath findinpath left review comments

@findepi findepi Awaiting requested review from findepi

Assignees
No one assigned
Labels
cla-signed docs
Milestone
444
Development

Successfully merging this pull request may close these issues.
3 participants
@anusudarsan @electrum @findinpath