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

Use S3 compatible driver for interacting with GCS #727

Merged
merged 5 commits into from
Jan 22, 2024
Merged

Use S3 compatible driver for interacting with GCS #727

merged 5 commits into from
Jan 22, 2024

Conversation

m90
Copy link
Contributor

@m90 m90 commented Jan 18, 2024

Copy link
Contributor

@deer-wmde deer-wmde left a comment

Choose a reason for hiding this comment

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

I'm missing some context but to me this looks like it's re-introducing the changes from #719 + ACL fixes for the problems we encountered, so I approve

@@ -1,10 +1,13 @@
# api

## 8x.34.2 - 18 January
- Use S3 driver for accessing static files in GCP buckets, using proper ACLs
Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to confirm I understand this correctly:

This applies exactly the same logic as #719 but also adds the 'visibility' => 'public' setting to all places we save files. For what it's worth I'd have no objection to also setting this for the whole Storage disk object since I can see us forgetting to set this in future and confusing us.

Indeed if I understood our possible desire to move towards Unified Bucket access (e.g. with no per object ACLs) perhaps we don't want to be having the s3 driver setting these things at at; we might want to advance to the 3.0 version (perhaps with a small feature outage while we do it). This would then allow Laravel to return the the state it was in before of "totally unaware of anything about the file visibility of uploaded logos".

These changes alone are not sufficient to make the feature work (i.e. logos won't be visible to the public) though since now the service account this is using needs to also have the right to make the ACL changes (since 1.0 of league/flysystem-aws-s3-v3) dies if it can't make the ACL changes it tries to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes alone are not sufficient to make the feature work (i.e. logos won't be visible to the public) though since now the service account this is using needs to also have the right to make the ACL changes (since 1.0 of league/flysystem-aws-s3-v3) dies if it can't make the ACL changes it tries to.

I'm not entirely sure what this paragraph is trying to say.

@m90 m90 merged commit 938c3c5 into main Jan 22, 2024
5 checks passed
@m90 m90 deleted the fr/gcp-to-s3-2 branch January 22, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants