Skip to content

[feature] Serve precompressed static files#653

Open
nemesifier wants to merge 2 commits into
masterfrom
features/560-serve-precompressed-files
Open

[feature] Serve precompressed static files#653
nemesifier wants to merge 2 commits into
masterfrom
features/560-serve-precompressed-files

Conversation

@nemesifier

@nemesifier nemesifier commented Jul 4, 2026

Copy link
Copy Markdown
Member

Summary

Adds support for serving precompressed static assets from the nginx container.

Changes

  • Builds and loads the nginx Brotli static module against the nginx.org package used by the official nginx Alpine image.
  • Enables brotli_static and moves gzip_static into the static file locations.
  • Uses direct alias locations for static files so nginx serves .br and .gz files directly instead of compressing the original file at request time.
  • Keeps /static/custom/ mapped separately so documented custom theme assets continue to work.
  • Removes obsolete Django static compression flags that are no longer used by the current storage backend.

Notes

The Brotli module is built in the image instead of installing Alpine's nginx-mod-http-brotli package because the official nginx image uses nginx.org packages, while Alpine module packages target Alpine's own nginx build.

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds Brotli static compression support to the Nginx Docker image. The Dockerfile now uses a multi-stage build to compile the ngx_brotli module against a parameterized nginx version and installs it into the runtime image. Nginx configuration templates load the brotli module and update static file serving to use alias-based locations with gzip_static/brotli_static directives, replacing prior try_files logic. Obsolete Django static compression constants are removed, and a new NGINX_BROTLI_SWITCH environment variable is documented.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Nginx
  participant StaticDir

  Client->>Nginx: GET /static/file.css
  Nginx->>Nginx: load_module ngx_http_brotli_static_module
  Nginx->>StaticDir: check for file.css.br (brotli_static)
  StaticDir-->>Nginx: precompressed file or fallback
  Nginx-->>Client: response (brotli/gzip/plain)
Loading

Related PRs: None identified.

Suggested labels: enhancement, nginx, docker

Suggested reviewers: nginx/infra maintainers familiar with Docker multi-stage builds and Nginx configuration templates.

Poem

A rabbit hopped through Nginx's den,
Compressed with Brotli, now and then,
Static files served light and fast,
Old gzip switches from the past,
Docs updated, modules load —
A speedier hop down static road. 🐇📦


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (2 errors, 2 warnings)

Check name Status Explanation Resolution
Bug Fixes ❌ Error Root cause is addressed in nginx config, but no regression test was added; the only existing Selenium static-file test doesn’t cover precompressed Brotli/gzip serving. Add a deterministic regression test (preferably Selenium or integration) that requests a static asset with Accept-Encoding and asserts the precompressed file is served; it should fail without the patch.
General Rules ❌ Error The PR changes nginx static serving and the Dockerfile, but adds no dedicated tests; existing Selenium tests only cover generic static assets, not brotli/gzip serving or module loading. Add regression tests for the nginx image/template changes: verify brotli module loading, /static and /static/custom aliasing, and precompressed asset serving with gzip/brotli enabled.
Features ⚠️ Warning Issue #560 exists and docs were updated, but the PR adds no tests or coverage evidence for the nginx/static-asset changes. Add functional tests for nginx serving precompressed gzip/brotli assets and custom static paths, then rerun coverage.
Changes ⚠️ Warning Docs were updated, but no tests were added for the new nginx Brotli/precompressed-static behavior; the PR only touches config/docs files. Add coverage for nginx config/static serving, e.g. validate brotli module loading and /static/ and /static/custom/ serving .br/.gz assets.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly matches the feature change and uses the required [type] prefix.
Description check ✅ Passed The description is substantive and on-topic, but it omits the checklist, issue reference, and screenshot sections from the template.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch features/560-serve-precompressed-files

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nemesifier nemesifier added the enhancement New feature or request label Jul 4, 2026
@nemesifier nemesifier self-assigned this Jul 4, 2026
@github-project-automation github-project-automation Bot moved this to Backlog in 26.06 Release Jul 4, 2026
@@ -1,7 +1,46 @@
FROM nginx:1.31.2-alpine

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@coderabbitai wouldn't this cause problems do dependabot?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🤖 web_search executed:

Does Dependabot support updating Docker FROM image version when using a global ARG variable for the tag in multi-stage builds?

Length of output: 9360


🏁 Script executed:

#!/bin/bash
fd -i dependabot .github -a
cat .github/dependabot.yml 2>/dev/null

Length of output: 1238


@nemesifier, good instinct to double-check, but this pattern should actually work fine with Dependabot.

Docker ecosystem is enabled for /images/openwisp_nginx in .github/dependabot.yml, so the Dockerfile is scanned.

The key thing here is that Dependabot added explicit "global ARG" support (dependabot-core#2057, landed via PR #4837) precisely for this pattern: an ARG with a default value declared before the first FROM, then redeclared inside each stage that needs it. That's exactly what this Dockerfile does:

ARG NGINX_VERSION=1.31.2
...
FROM nginx:${NGINX_VERSION}-alpine AS brotli-builder
ARG NGINX_VERSION
...
FROM nginx:${NGINX_VERSION}-alpine
ARG NGINX_VERSION

Since both FROM lines resolve the same NGINX_VERSION variable (not two independently hardcoded tags), Dependabot's parser treats it as a single dependency and only needs to bump the one ARG NGINX_VERSION=1.31.2 line — it won't try to open separate/conflicting PRs for each FROM. That "two FROM directives" problem (dependabot-core#6700) mainly affects Dockerfiles where each FROM hardcodes its own version string independently (e.g., nginx:1.19.9 and nginx:1.19.9-alpine written separately) — that's not the case here since both stages share the same ARG-driven tag.

So, this should be safe — Dependabot will detect the ARG NGINX_VERSION=1.31.2 line and bump it, updating both stages consistently in a single PR.

@nemesifier nemesifier moved this from Backlog to Ready for review/testing in 26.06 Release Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Ready for review/testing

Development

Successfully merging this pull request may close these issues.

1 participant