Port to Django 5.2 - #635
FreneticScribbler wants to merge 23 commits into
Conversation
Any newer and the mystery importlib metadata error appears. Weird! >_>
Having two distinct test flavours is giving me a headache
Makes a VERY big image, I suspect we can optimise this a lot...
![code-review-doctor[bot]](https://avatars.githubusercontent.com/in/83430?s=60&v=4){kind=small}
| 'NAME': str(BASE_DIR / 'db.sqlite3'), | ||
| } | ||
| 'default': { | ||
| 'ENGINE': 'django.db.backends.{}'.format( |
There was a problem hiding this comment.
f-string is easier to read, write, and less computationally expensive than legacy string formatting. Explained here.
There was a problem hiding this comment.
oh shut up you
|
Should look at updating python from 3.10 at some point too. And probably defining it in a single source of truth - currently it's manually defined in the dockerfile |
still got lots of failed tests :(
…ve test stability * Upgrade Python from 3.10 to 3.12 * Update frontend dependencies, replace node-sass with sass
- Add Nginx as a reverse proxy - Add cert-selfsign.sh for generating self-signed certificates - Add certbot-issue.sh for Let's Encrypt certificate issuance and renewal - Add cron-install.sh and cron-uninstall.sh for system cron management - Add .env.example as an environment variable template
jb3
left a comment
jb3
left a comment
There was a problem hiding this comment.
Overall looks good, I haven't yet tested this properly locally so the below is just a code review.
There may be some other bits that present themselves when trying to get this running but this is just a preliminary pass.
| CSRF_TRUSTED_ORIGINS = [f"http://{host}" for host in ALLOWED_HOSTS] | ||
| CSRF_TRUSTED_ORIGINS.append("http://localhost:8000") | ||
| CSRF_TRUSTED_ORIGINS.append("http://localhost:8001") | ||
| ALLOWED_HOSTS = ['*'] |
There was a problem hiding this comment.
We probably want some of these CSRF variables for production, there were behaviour changes here in Django 5.
| cp -fL "\$RENEWED_LINEAGE/fullchain.pem" "$CERTS_DIR/fullchain.pem" | ||
| cp -fL "\$RENEWED_LINEAGE/privkey.pem" "$CERTS_DIR/privkey.pem" | ||
| chmod 600 "$CERTS_DIR/privkey.pem" | ||
| docker compose -f "$REPO_DIR/compose.yml" exec -T nginx nginx -s reload || true |
There was a problem hiding this comment.
We probably don't want to silently ignore this? Why have we got || true?
| nginx/ | ||
| *.pem | ||
| *.key | ||
| .env.* |
There was a problem hiding this comment.
We probably want to catch .env generally, not just .env.*, secrets should never be in container images.
There was a problem hiding this comment.
This is a little bit unwieldy.
Have you considered instead excluding everything (*) and then using inverse operators to explicitly include the files that you do wish to be in the container?
| --ignore=RIGS/tests/test_interaction.py \ | ||
| --ignore=assets/tests/test_interaction.py \ | ||
| --ignore=training/tests/test_interaction.py |
There was a problem hiding this comment.
Shouldn't really be done here, we should either remove those tests files if they are just totally useless or fix them. We shouldn't be making excludes like this at the CI workflow level.
| @@ -0,0 +1,54 @@ | |||
| # Stage 1: Base build stage | |||
| FROM combos/python_node:3.12_22 AS base | |||
There was a problem hiding this comment.
Can we not use any later version than Python 3.12 and Node 22?
Node 22 is already in maintenance and nearing EOL, and given the update frequency to RIGS it'd be nice to be on a longer supported Python version as well.
| # import debug_toolbar | ||
| urlpatterns += [ | ||
| path('__debug__/', include(debug_toolbar.urls)), | ||
| # path('__debug__/', include(debug_toolbar.urls)), |
There was a problem hiding this comment.
May as well be removed or locked behind an env var, no point leaving this as commented out code.
| "asgiref", | ||
| "beautifulsoup4", | ||
| "Brotli", | ||
| "dj-database-url", |
There was a problem hiding this comment.
I think the usage of this is also removed.
| from assets import models as asset_models | ||
| from training import models as training_models | ||
|
|
||
| # Template context processor |
There was a problem hiding this comment.
Not really a helpful comment.
Either remove it or add it as a docstring to what you're actually describing (multiline """ string at start of function).
| # Template context processor | ||
|
|
||
|
|
||
| def is_ajax(request): |
There was a problem hiding this comment.
Highlighted elsewhere with the couple of instances I spotted, but worth double checking all calls of is_ajax.
This method was previously assumed to return a boolean, always returning a non-empty dict means that for ALL previous callers the return is effectively True (as a non-empty dict is truthy in Python). This is a pretty large API change and so is worth double checking all your calls.
3 participants
Dockerfile is not very well optimised, it currently produces a 1.66GB image!!
Probably think about switching to using docker / postgres DB in development workflow, using sqlite has caused owt but problems in the past. Some work has been done to that end in this PR, much remains.
Caveat emptor