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

Tune cloud storage upload params and tidy settings #14711

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

stevejalim
Copy link
Collaborator

@stevejalim stevejalim commented Jun 20, 2024

This changeset slightly refactors the ordering of some of our settings related to storage, to help reduce the risk of misintepretation -- having STORAGES tweaked way after they are initially defined in the file was a potential future footgun.

Also, this changeset starts to use MEDIA_URL (which is defined in the k8s setup to point to the assets bucket) for serving images that have been uploaded via the CMS via the CDN. This will mean uploaded images will come from, say, www.mozilla.org/media avoiding potential cookie-related issues.

We also make sure that images uploaded into the storage bucket go into a separate dir that won't clash with anything copied over from Bedrock's hard-coded assets, and now have a filepath that makes it obvious they are custom uploads.

Finally, I also checked whether forms that use file uploads (eg legal trademarks form) will be affected by us changing STORAGES["default"] to point to a cloud bucket. It's a non-issue: such files don't get put into permanent storage, and instead are held as temporary files in memory or spooled to /tmp/ (docs)

@stevejalim stevejalim changed the title Tune gcs auth params for cms Tune cloud storage upload params and tidy settings Jun 20, 2024
if GS_BUCKET_NAME and GS_PROJECT_ID:
GS_CUSTOM_ENDPOINT = MEDIA_URL # hostname that proxies the storage bucket
GS_FILE_OVERWRITE = False
GS_LOCATION = "custom-media" # path within the bucket to upload to (and used when generating URLs)
Copy link
Collaborator Author

@stevejalim stevejalim Jun 20, 2024

Choose a reason for hiding this comment

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

GS_LOCATION and GS_CUSTOM_ENDPOINT are additions here - the rest is shuffling.

you can see docs for these settings at https://django-storages.readthedocs.io/en/latest/backends/gcloud.html

@stevejalim stevejalim force-pushed the tune-gcs-auth-params-for-cms branch from 4b5113c to 70c6e1c Compare June 20, 2024 09:53
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 77.16%. Comparing base (42d095d) to head (70c6e1c).

Files Patch % Lines
bedrock/settings/base.py 55.55% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14711      +/-   ##
==========================================
- Coverage   77.17%   77.16%   -0.01%     
==========================================
  Files         159      159              
  Lines        8245     8246       +1     
==========================================
  Hits         6363     6363              
- Misses       1882     1883       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@alexgibson alexgibson left a comment

Choose a reason for hiding this comment

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

The changes make sense to me and things seem to run as usual when I test locally (although I don't really know how to test it more fully yet). If we're confident that the legal trademarks form should be unaffected then falling back to local filesystem storage makes sense to me. A code-level r+ from me.

@stevejalim stevejalim merged commit cc12081 into main Jun 20, 2024
5 checks passed
@stevejalim stevejalim deleted the tune-gcs-auth-params-for-cms branch June 20, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants