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

add s3 support #1197

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

add s3 support #1197

wants to merge 7 commits into from

Conversation

slimani-dev
Copy link

(optional) Issue number:

Summary of the change:

I have changed the image URL to use temporary URLs if the selected storage is s3

@jonnott
Copy link
Contributor

jonnott commented Aug 24, 2024

@streamtw This would be amazing to have merged, so private S3 buckets can be used..

@streamtw
Copy link
Member

streamtw commented Aug 26, 2024

Thanks for the info and this PR. I don't have a S3 bucket now. But I will sure apply one and test this PR before merging. Cannot garuantee a specific date though.

@streamtw streamtw added the WIP Working in progress. label Aug 26, 2024
@jonnott
Copy link
Contributor

jonnott commented Aug 26, 2024

Thanks for the info and this PR. I don't have a S3 bucket now. But I will sure apply one and test this PR before merging. Cannot garuantee a specific date though.

Thanks.

Seems the temporaryUrl() thing is needed for private S3 buckets. Also, we've been unable to make the thumbnails load from S3 buckets.

Might be worth looking at other PRs and issues around S3 at the same time, as it'd be good to get S3 compatibility fully fixed.

We are currently facing having to try and move away from this package because we have a large multi-tenant multi-server app which needs to use per-tenant S3 buckets for all stored files.

@jonnott
Copy link
Contributor

jonnott commented Oct 8, 2024

Thanks for the info and this PR. I don't have a S3 bucket now. But I will sure apply one and test this PR before merging. Cannot garuantee a specific date though.

@streamtw Did you get anywhere with this?

@et-hoangdv
Copy link

Ok this PR which remove those timestamp query string should to fix my issue: #1248

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Working in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants