Upload headshots to S3 via django-storages #1134

hancush · 2024-07-08T20:53:37Z

Overview

This PR makes use of an upstream change to django-councilmatic allowing the headshot storage backend to be configured by Councilmatic instances. When credentials are provided, it configures the Metro app to store and retrieve headshots from S3.

This PR also performs some housekeeping (#1055):

Clean up files from old deployment strategies
Update README to reflect current stack and development practices
Document environment variables in app.json
Default environment variables to development settings

Connects #1073

Notes

Depends on new release of django-councilmatic. Current draft pinned to branch with change.

datamade/django-councilmatic#292

Testing Instructions

Log in to the review app using the staging credentials in Bitwarden.
View the board member listing and confirm headshots are rendering. (Most are placeholders. Janice Hahn has a real one.)
Navigate to a board member and upload a new headshot. Confirm that it appears on their detail page and the listing page.

hancush · 2024-07-09T14:52:33Z

requirements.txt

@@ -1,4 +1,5 @@
-django-councilmatic[convert_docs]>=3.0
+# django-councilmatic[convert_docs]>=3.0
+git+https://github.com/datamade/django-councilmatic.git@hcg/configurable-headshot-backend#egg=django-councilmatic


TODO: Update this to new django-councilmatic release once it becomes available.

hancush · 2024-07-09T15:40:29Z

Dockerfile

 FROM python:3.10-slim-bullseye
 LABEL maintainer "DataMade <[email protected]>"

 RUN apt-get update && \
    apt-get install -y libpq-dev gcc gdal-bin gnupg && \
    apt-get install -y libxml2-dev libxslt1-dev antiword unrtf poppler-utils \
                       tesseract-ocr flac ffmpeg lame libmad0 libsox-fmt-mp3 \
-                       sox libjpeg-dev swig libpulse-dev curl && \
+                       sox libjpeg-dev swig libpulse-dev curl git && \


TODO: Need git to install django-councilmatic from the branch, will remove when new release is cut.

hancush · 2024-07-09T15:41:21Z

councilmatic/settings.py

@@ -8,8 +8,33 @@

 env = environ.Env(
    # Set default values
-    DEBUG=(bool, False),
-    LOCAL_DOCKER=(bool, False),
+    LOCAL_DOCKER=(bool, True),


We default to development settings in other apps. Think it's a good idea to preserve this pattern and keep the size of the local env file small, since these are overridden in deployment already.

hancush · 2024-07-09T15:41:42Z

councilmatic/settings.py

@@ -19,7 +44,7 @@
 environ.Env.read_env(os.path.join(BASE_DIR, ".env.local"))

 SECRET_KEY = env("DJANGO_SECRET_KEY")
-DEBUG = env.bool("DEBUG")
+DEBUG = env.bool("DJANGO_DEBUG")


It's conventional to prefix settings with the app they're relevant to.

hancush · 2024-07-09T15:42:15Z

councilmatic/settings.py

+AWS_S3_ACCESS_KEY_ID = env("AWS_S3_ACCESS_KEY_ID")
+AWS_S3_SECRET_ACCESS_KEY = env("AWS_S3_SECRET_ACCESS_KEY")
+AWS_STORAGE_BUCKET_NAME = env("AWS_STORAGE_BUCKET_NAME")


Django Storages automatically discovers these variables.

neat! that makes things easier. if we're removing these non-S3 specific vars here, what do you think of these heroku config var adjustments as well?

staging - in addition to AWS_KEY and AWS_SECRET, there's also AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY which seem to hold duplicate values of their S3 counterparts. i imagine these can all be removed

production - the S3 versions do not exist, so we can likely change AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY to match the S3 var names. django storages also finds these automatically, but i'm unsure if the mismatch would cause issues considering these settings are looking for the S3 specific versions.

review apps are using only the S3 specific variables, so no change there

hancush · 2024-07-09T15:44:37Z

lametro/models.py

-            # Returning here solves that.
-            if "headshot_placeholder.png" not in self.headshot.path:
-                return self.headshot
+            return self.headshot.url


We previously needed to load all images with static() because django-councilmatic was storing them with static files. Now, since we've told django-councilmatic to upload headshots to S3, if there is a headshot, we can return its URL directly, and only get the manual headshots and placeholder from static.

hancush · 2024-07-09T15:45:09Z

lametro/views.py

@@ -668,7 +667,7 @@ def post(self, request, *args, **kwargs):
                    self.get_context_data(form=form, headshot_error=error)
                )

-            person.headshot = self.get_file_url(request, file_obj)
+            person.headshot = file_obj


person.headshot is a file field, so we can pass it this file directly.

xmedr

the README updates and env var cleanup are very much appreciated! and the headshot uploads work as expected. just a couple questions inline

xmedr · 2024-07-10T16:12:23Z

councilmatic/settings.py

+AWS_S3_ACCESS_KEY_ID = env("AWS_S3_ACCESS_KEY_ID")
+AWS_S3_SECRET_ACCESS_KEY = env("AWS_S3_SECRET_ACCESS_KEY")
+AWS_STORAGE_BUCKET_NAME = env("AWS_STORAGE_BUCKET_NAME")


neat! that makes things easier. if we're removing these non-S3 specific vars here, what do you think of these heroku config var adjustments as well?

staging - in addition to AWS_KEY and AWS_SECRET, there's also AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY which seem to hold duplicate values of their S3 counterparts. i imagine these can all be removed

production - the S3 versions do not exist, so we can likely change AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY to match the S3 var names. django storages also finds these automatically, but i'm unsure if the mismatch would cause issues considering these settings are looking for the S3 specific versions.

review apps are using only the S3 specific variables, so no change there

xmedr · 2024-07-10T16:14:06Z

README.md

line 17 here refers to a legacy way of running the app without docker. is this still useful to us? those old docs talk about using Solr which has been removed here, so they're also out of date. If we want to keep the reference to the old magicks, we could maybe change the language to something like:

These days, we run apps in containers for local development. More on that here. For reference, if you'd like to read up on how we used to run this app locally, see the legacy setup instructions.

The line in question:

la-metro-councilmatic/README.md

Line 17 in 04df15e

These days, we run apps in containers for local development. More on that [here](https://github.com/datamade/how-to/docker/local-development.md). Prefer to run the app locally? See the [legacy setup instructions](https://github.com/datamade/la-metro-councilmatic/blob/b8bc14f6d90f1b05e24b5076b1bfcd5e0d37527a/README.md).

Great call. Since we don't run apps without Docker any more, I just removed the reference to the old setup instructions.

hancush · 2024-07-10T18:17:29Z

@xmedr Yes, those config var changes are a good idea. Best to have everything standardized.

antidipyramid

Works great. Thanks for also including the README updates!

xmedr

sweet, i just made those config var changes for prod and staging, so this looks good to me!

hancush added 7 commits July 8, 2024 12:25

Clean up README, unused files, AWS settings

c075b35

Use django-storages to handle headshot uploads

f6fb20d

Pin to updated django-councilmatic lib

ccbc3dd

Add logging when AWS configured

c8340e5

Remove custom storage

afe2d5e

Copy .env file for testing

87f67e9

Undo unneeded changes

2b7faba

fgregg temporarily deployed to la-metro-cou-hcg-headsh-5e7gzb July 8, 2024 21:08 Inactive

Add git for install, update requirements.txt

7a3e127

fgregg temporarily deployed to la-metro-cou-hcg-headsh-hwoimk July 8, 2024 21:17 Inactive

Remove references to manual headshots and bios in the README

bb3ae93

hancush temporarily deployed to la-metro-cou-hcg-headsh-hwoimk July 8, 2024 21:18 Inactive

Remove blackbox from Metro image

e2fa08d

hancush temporarily deployed to la-metro-cou-hcg-headsh-hwoimk July 8, 2024 21:20 Inactive

hancush added 2 commits July 9, 2024 09:31

Add documentation of env vars to app.json

66b9813

Remove trailing commas from JSON

0031438

hancush commented Jul 9, 2024

View reviewed changes

fgregg temporarily deployed to la-metro-cou-hcg-headsh-3ut4cn July 9, 2024 14:53 Inactive

Default to local development settings

3ecb2ca

hancush temporarily deployed to la-metro-cou-hcg-headsh-3ut4cn July 9, 2024 15:19 Inactive

Default SHOW_TEST_EVENTS to False

2d61285

hancush temporarily deployed to la-metro-cou-hcg-headsh-3ut4cn July 9, 2024 15:24 Inactive

Remove Postgres from review apps

aab9e4c

hancush temporarily deployed to la-metro-cou-hcg-headsh-3ut4cn July 9, 2024 15:32 Inactive

hancush commented Jul 9, 2024

View reviewed changes

hancush marked this pull request as ready for review July 9, 2024 15:45

hancush requested review from xmedr and antidipyramid July 9, 2024 15:45

hancush mentioned this pull request Jul 9, 2024

Make headshot backend configurable datamade/django-councilmatic#292

Merged

xmedr requested changes Jul 10, 2024

View reviewed changes

Strike some more legacy info from the README

99918fa

hancush temporarily deployed to la-metro-cou-hcg-headsh-3ut4cn July 10, 2024 18:17 Inactive

antidipyramid approved these changes Jul 10, 2024

View reviewed changes

xmedr approved these changes Jul 15, 2024

View reviewed changes

Update django-councilmatic version

3c00ced

hancush temporarily deployed to la-metro-cou-hcg-headsh-3ut4cn July 15, 2024 15:44 Inactive

hancush merged commit 16a70ba into main Jul 15, 2024
2 checks passed

hancush mentioned this pull request Jul 15, 2024

Remove documentation and code related to the old deployment pattern #1055

Closed

antidipyramid mentioned this pull request Jul 16, 2024

Restore AWS_KEY and AWS_SECRET env vars #1136

Merged

antidipyramid mentioned this pull request Jul 23, 2024

Headshot Upload Error #1122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upload headshots to S3 via django-storages #1134

Upload headshots to S3 via django-storages #1134

hancush commented Jul 8, 2024 •

edited

Loading

hancush Jul 9, 2024

hancush Jul 9, 2024 •

edited

Loading

hancush Jul 9, 2024

hancush Jul 9, 2024

hancush Jul 9, 2024

xmedr Jul 10, 2024

hancush Jul 9, 2024

hancush Jul 9, 2024

xmedr left a comment

xmedr Jul 10, 2024

xmedr Jul 10, 2024

hancush Jul 10, 2024

hancush commented Jul 10, 2024

antidipyramid left a comment

xmedr left a comment

Upload headshots to S3 via django-storages #1134

Upload headshots to S3 via django-storages #1134

Conversation

hancush commented Jul 8, 2024 • edited Loading

Overview

Notes

Testing Instructions

Choose a reason for hiding this comment

hancush Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmedr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hancush commented Jul 10, 2024

antidipyramid left a comment

Choose a reason for hiding this comment

xmedr left a comment

Choose a reason for hiding this comment

hancush commented Jul 8, 2024 •

edited

Loading

hancush Jul 9, 2024 •

edited

Loading