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: set s3_acl_enabled to false when s3_permission empty #17979

Closed
wants to merge 1 commit into from

Conversation

everpcpc
Copy link

@everpcpc everpcpc commented Apr 7, 2022

for: #17978

A kt-paperclip upgrade is also needed, e.g. kreeti/kt-paperclip#80

@shleeable
Copy link
Contributor

If this sets any kind of default. Please add it to the .env.production file

@Gargron
Copy link
Member

Gargron commented Apr 7, 2022

What is the replacement for ACL? Setting media to private on suspension is a pretty important function.

@everpcpc
Copy link
Author

everpcpc commented Apr 7, 2022

What is the replacement for ACL? Setting media to private on suspension is a pretty important function.

there is a Bucket owner enforced mode to use policy for bucket
https://docs.aws.amazon.com/AmazonS3/latest/userguide/about-object-ownership.html

@Gargron
Copy link
Member

Gargron commented Apr 7, 2022

Oh, so AWS is not removing support for ACL, it is a custom setting.

@stale
Copy link

stale bot commented Aug 13, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status/wontfix This will not be worked on label Aug 13, 2022
@everpcpc
Copy link
Author

What is the replacement for ACL? Setting media to private on suspension is a pretty important function.

Since we usually want our s3 bucket to be private entirely and serve files via CloudFront, maybe we could change this way suspension media. For example by moving them to specific path with special bucket or CloudFront policy?

@lgE-1
Copy link

lgE-1 commented Oct 22, 2022

This option is also necessary for other s3 services which do not support ACLs such as Cloudflare R2.

@everpcpc
Copy link
Author

kreeti/kt-paperclip#92
has been merge, and we need to wait for this commit release

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

This pull request has merge conflicts that must be resolved before it can be merged.

@mjankowski
Copy link
Contributor

It looks like that kt-paperclip change was merged and released in v7.2.0, which we are upgraded past now.

If this is still relevant, can you rebase? If not, close.

@everpcpc
Copy link
Author

everpcpc commented Dec 5, 2023

Seems like we can just set this option with S3_PERMISSION, update.

@everpcpc everpcpc changed the title feat: add env option S3_ACL_DISABLED fix: set s3_acl_enabled to false when s3_permission empty Dec 5, 2023
Copy link
Contributor

github-actions bot commented Dec 5, 2023

This pull request has resolved merge conflicts and is ready for review.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@ThisIsMissEm
Copy link
Contributor

This currently has a "won't fix" label, though it looks like it was automatically added — is this still something that's necessary or desired?

@mjankowski
Copy link
Contributor

Based on rebase needed and sheer age - closing this. Please rebase/reopen if still relevant.

@mjankowski mjankowski closed this Nov 20, 2024
@everpcpc
Copy link
Author

everpcpc commented Dec 5, 2024

I've rebased this, but I did not see any reopen button

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rebase needed 🚧 status/wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants