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

Upgrade to Django 5 #266

Open
wants to merge 10 commits into
base: optimise-docker-build
Choose a base branch
from
Open

Conversation

SteveMarshall
Copy link
Member

This jumps us all the way from Django 1.11 to Django 5. Everything appears to work, with only minimal usage of deprecation utils for middleware.

settings.STATICFILES_DIGEST_FREE_STORAGE,
)()

staticfiles_digest_free_storage = storages["staticfiles"]
Copy link
Member

Choose a reason for hiding this comment

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

Erm, this is no longer a digest free storage? It's now using whitenoise when previously we separated this storage from the default storage deliberately - although I can't remember the details. The commit message should explain why this change is happening because it's a more subtle one. Ideally it would reference back to the commit that introduced the separation to enable review without manual spelunking.

Copy link
Member

Choose a reason for hiding this comment

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

(Beyond that LGTM.)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a great spot, thanks @jaylett! I don't think I'd clocked the difference because I was mostly just trying to get to a state where I could run the site, sort of thing.

I've reinstated the correct behaviour in be706b3 (spotting issues starting Gunicorn and serving CSS in production along the way, resolved in d88f1ed and da4829d respectively), and will explain the change better in the resultant commit, linking to 0fa3926 (where digest-free statics were introduced) as you suggest, along with linking to WhiteNoise's documentation about the two storage backends.

@SteveMarshall SteveMarshall force-pushed the optimise-docker-build branch 2 times, most recently from 698273c to 70806e7 Compare April 1, 2024 09:19
@SteveMarshall SteveMarshall changed the base branch from optimise-docker-build to main April 1, 2024 13:33
@SteveMarshall SteveMarshall changed the base branch from main to optimise-docker-build April 1, 2024 13:33
Django 5 requires Python 3.10 and, as we're using the OS-supplied
Python, that means upgrading to a more recent Ubuntu (no bad thing).
That also requires that we upgrade pyScss to something more recent.

This definitely won't work, because we haven't actually upgraded any of
our code to be Django 5 compatible.
This is the result of applying the `django-upgrade` tool
(https://github.com/adamchainz/django-upgrade) to our entire codebase:

`git ls-files -z -- '*.py' |
  xargs -0 django-upgrade --target-version 5.0`

It fixes a few things, but not everything.
This makes our existing middleware Django 5 compatible using the
MiddlewareMixin, per
https://docs.djangoproject.com/en/5.0/topics/http/middleware/#upgrading-pre-django-1-10-style-middleware.

We'll want to upgrade these at some point, but this works for now.
For reasons I can't entirely get to the bottom of, the particular
combination of Ubuntu and Django that we're using don't natively
use the system timezones as they did in earlier versions. The `tzdata`
package manages that for systems where it doesn't just work.
`staticfiles` is now `static`, and `get_storage_class` has been replaced
by `storages`.
`render_to_response` was deprecated in Django 2, and replaced with
`render`.
For a long time, Django has supported this more natural syntax for
conditionals, so now we can use it too!
Without upgrading gunicorn (and, by extension, eventlet and greenlet),
gunicorn won't start, complaining of a syntax error inside gunicorn. I
didn't catch this earlier in this branch because I wasn't running in
production mode.
Whitenoise 4.x has a bug when used with Django 3+ where the former
serves a `Content-Disposition` header that makes Safari think that it
doesn't need to (or can't) ungzip the response, causing the assets to
fail to parse. This is described in (slightly) more detail here:
evansd/whitenoise#241, and was resolved in
WhiteNoise 5.
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.

None yet

2 participants