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

Staticfiles fix #715

Closed
wants to merge 9 commits into from
Closed

Staticfiles fix #715

wants to merge 9 commits into from

Conversation

kcaze
Copy link
Collaborator

@kcaze kcaze commented Dec 21, 2023

It looks like there were 2 main issues with static files:

  1. The staticfiles configs we set were getting clobbered by django_heroku.settings(). Setting staticfiles=False means we're using our own settings.
  2. The staticfile config we did have wasn't correct. It was missing STATIC_ROOT and the backend storage config was also incorrect. See https://whitenoise.readthedocs.io/en/latest/django.html for details.

I also updated docker-entrypoint.sh to use the --no-input flag when running collectstatic.

Copy link
Contributor

@akirabaruah akirabaruah left a comment

Choose a reason for hiding this comment

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

Dope 😎 Thanks for the fix! Had some minor questions but otherwise LGTM

"staticfiles": {
"BACKEND": "whitenoise.storage.CompressedManifestStaticFilesStorage",
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change required? Or is it basically the "new way" to do things? (in which case, seems reasonable to switch)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tested with this syntax vs the other and it seems necessary. According to the docs, this is needed for Django 4.2+

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. FWIW, I think Django is still 4.1 in the poetry.lock file.

@@ -172,7 +177,7 @@
# Configure Django App for Heroku.
import django_heroku

django_heroku.settings(locals(), test_runner=False)
django_heroku.settings(locals(), test_runner=False, staticfiles=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a comment above this line pointing out that staticfiles=False is to avoid clobbering previous settings?

Copy link
Contributor

Choose a reason for hiding this comment

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

Where did this change come from?

STATIC_URL = "/static/"
STATICFILES_DIRS = [
os.path.join(BASE_DIR, "cardboard/static"),
os.path.join(BASE_DIR, "hunts/static"),
]
STATICFILES_STORAGE = "whitenoise.django.CompressedManifestStaticFilesStorage"
STORAGE = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a typo, this setting is named STORAGES https://docs.djangoproject.com/en/5.0/ref/settings/#storages

But it was also added in Django 4.2. In the pyproject.toml we only have 4.1 specified and the package version in the poetry.lock is also 4.1, so you should use the 4.1 settings (we should also upgrade Django to the latest version but probably in a separate PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh oops, you're right. I'll delete my staticfiles directory and test from scratch again with this fixed.

@@ -172,7 +177,7 @@
# Configure Django App for Heroku.
import django_heroku

django_heroku.settings(locals(), test_runner=False)
django_heroku.settings(locals(), test_runner=False, staticfiles=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did this change come from?

@rgossiaux
Copy link
Contributor

I think our settings are working ok and django_heroku isn't doing anything too nefarious to them; you can see what changes it makes here: https://github.com/heroku/django-heroku/blob/master/django_heroku/core.py We have a few redundant lines in this file but nothing in conflict.

With that said we should probably just rip out that module wholesale and inline everything. That's what the creator recommends; looks like it's been deprecated for a couple years: heroku/django-heroku#56

@akirabaruah
Copy link
Contributor

Now that we have the webpack fix in #716, we can prob close this PR and followup with a separate django-heroku cleanup one?

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.

3 participants