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

specify acl for S3 objects #412

Merged
merged 7 commits into from
Aug 12, 2022
Merged

Conversation

ivan-aws
Copy link
Contributor

Allows to specify the object ACL when using the S3 module. Uses private as the default, which was the implicit default before.

This allows to utilize different ACLs, like bucket-owner-full-control which is useful when operating with shared files.

Example:

s3 = S3(<region>,<bucket>)

s3.put_object(
    s3_asset_key,
    file_asset_path,
    style="s3-url",
    pre_check=True,
    object_acl="bucket-owner-full-control",
)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ivan-aws
Copy link
Contributor Author

Updated tests in second commit. tests pass locally for me.

Copy link
Collaborator

@sbkok sbkok left a comment

Choose a reason for hiding this comment

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

I've added a few style suggestions, nothing major.
Thanks for contributing this.

Done!
As an alternative, may I recommend switching to an opinionated formatting tool, such as black? Formatting can then be automated as part of the PR process.

Co-authored-by: Simon Kok <[email protected]>
@ivan-aws
Copy link
Contributor Author

Done!
As an alternative, may I recommend switching to an opinionated formatting tool, such as black? Formatting can then be automated as part of the PR process

@ivan-aws ivan-aws requested a review from sbkok December 28, 2021 12:20
@sbkok
Copy link
Collaborator

sbkok commented Dec 28, 2021

Actually, I am working on adding automated formatting.
However, since that changes almost every line in our repository, I am waiting for our current PRs to be merged first.
Such that we don't need to modify as many PRs.

So most likely around releasing 3.2.0 I will add that to the repository.

sbkok
sbkok previously approved these changes Dec 28, 2021
Copy link
Collaborator

@sbkok sbkok left a comment

Choose a reason for hiding this comment

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

LGTM!

@sbkok sbkok requested a review from deltagarrett December 28, 2021 12:42
@sbkok sbkok added this to the v3.2.0 milestone Dec 28, 2021
@sbkok sbkok added the enhancement New feature or request label Dec 28, 2021
@ivan-aws ivan-aws requested a review from sbkok March 24, 2022 17:07
Copy link
Collaborator

@sbkok sbkok left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@javydekoning javydekoning self-requested a review August 11, 2022 08:57
Copy link
Contributor

@javydekoning javydekoning left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@sbkok sbkok merged commit 7fd1d28 into awslabs:master Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants