[feature] Configured nginx to serve precompressed files (gzip/brotli) #599#601
Conversation
…on settings openwisp#599 Remove GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION settings from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst. Fixes openwisp#599
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two boolean vars to OpenWISP2 Nginx SSL config: Sequence Diagram(s)sequenceDiagram
participant Ansible as "Ansible Controller"
participant Apt as "Package Manager (apt)"
participant Nginx as "Nginx"
participant FS as "Filesystem (precompressed files)"
participant Client as "HTTP Client"
Ansible->>Apt: Install nginx + gzip/brotli modules (conditional)
Apt-->>Ansible: Install result (errors ignored)
Ansible->>Nginx: Render site-conf.j2 with gzip_static/brotli_static based on vars
Ansible->>Nginx: Reload/restart Nginx
Client->>Nginx: Request static asset (e.g., asset.js)
Nginx->>FS: Check for precompressed variants (asset.js.gz / asset.js.br)
alt precompressed exists and directive enabled
Nginx-->>Client: Serve precompressed file with appropriate Content-Encoding
else
Nginx-->>Client: Serve regular asset
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@defaults/main.yml`:
- Around line 163-164: The template unconditionally emits brotli_static which
breaks nginx if ngx_brotli isn't present; update the role to either install
ngx_brotli when brotli_static is "on" or make the template conditional: add a
boolean var like brotli_module_installed (default false) and modify the
site-conf.j2 to only render the brotli_static line when brotli_module_installed
is true and openwisp2_nginx_ssl_config.brotli_static == "on"; alternatively,
make the role install/enable ngx_brotli when
openwisp2_nginx_ssl_config.brotli_static is "on" so the directive is safe to
emit.
🧹 Nitpick comments (1)
docs/user/role-variables.rst (1)
165-166: Add a short note about the precompression prerequisite.These directives only serve precompressed assets when
.gz/.brfiles exist. A brief note in this section will prevent confusion for users who expect on-the-fly compression.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
defaults/main.ymldocs/user/role-variables.rsttemplates/openwisp2/settings.py
💤 Files with no reviewable changes (1)
- templates/openwisp2/settings.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Agent
- GitHub Check: Build debian13
- GitHub Check: Build ubuntu2204
- GitHub Check: Build ubuntu2404
- GitHub Check: Build debian12
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Pull request overview
This PR removes obsolete Django compression settings (GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION) that are no longer valid with the updated openwisp-utils package, and configures nginx to serve pre-compressed files directly using gzip_static and brotli_static directives. This shifts compression responsibility from Django to nginx for improved performance.
Changes:
- Removed deprecated Django compression settings from settings.py template
- Added gzip_static and brotli_static nginx directives to default configuration
- Updated documentation to reflect new nginx compression configuration
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| templates/openwisp2/settings.py | Removed obsolete GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION Django settings |
| defaults/main.yml | Added gzip_static and brotli_static directives to openwisp2_nginx_ssl_config |
| docs/user/role-variables.rst | Updated documentation with new nginx compression directives |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - "application/x-font-ttf" | ||
| - "font/opentype" | ||
| gzip_static: "on" | ||
| brotli_static: "on" |
There was a problem hiding this comment.
The brotli_static directive requires the nginx brotli module to be installed, but this module is not included in standard nginx packages. Without installing libnginx-mod-brotli (Debian/Ubuntu) or building nginx with the brotli module, this directive will cause nginx to fail with an "unknown directive" error. You need to either add a task to install the brotli module package in tasks/apt.yml or remove this directive until the module is installed.
| brotli_static: "on" |
| - "application/x-font-ttf" | ||
| - "font/opentype" | ||
| gzip_static: "on" | ||
| brotli_static: "on" |
There was a problem hiding this comment.
The brotli_static directive requires the nginx brotli module to be installed, but this module is not included in standard nginx packages. Without installing libnginx-mod-brotli (Debian/Ubuntu) or building nginx with the brotli module, this directive will cause nginx to fail with an "unknown directive" error. You need to either add a task to install the brotli module package in tasks/apt.yml or remove this directive until the module is installed.
| brotli_static: "on" |
…on settings openwisp#599 Remove GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION settings from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst. Fixes openwisp#599
…on settings openwisp#599 Remove GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION settings from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst. Fixes openwisp#599
|
hi @nemesifier the checks are failing even if there are no changes in the repo. The failing test appears to be a CI environment issue rather than a code problem. Could you please advise on how to proceed? |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@SAMurai-16 when the CI build does not fail due to your changes, the best thing is to:
- don't fight it in this PR, do your changes as if the CI Build is not failing
- open another PR which aims to A) Demonstrate the build fails on master, B) Help to debug it and fix it is always welcome.
…on settings openwisp#599 Remove GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION settings from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst. Fixes openwisp#599
@nemesifier i have raised an issue regarding this, do check out #602 |
…on settings openwisp#599 Remove GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION settings from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst. Fixes openwisp#599
nemesifier
changed the title
nemesifier
changed the title
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| - "application/x-font-ttf" | ||
| - "font/opentype" | ||
| # Enable precompressed gzip files serving (requires nginx with gzip_static module) | ||
| openwisp2_nginx_gzip_static: false |
There was a problem hiding this comment.
WARNING: Missing tests for new configuration variables
No tests have been added to verify that the new openwisp2_nginx_gzip_static and openwisp2_nginx_brotli_static variables are correctly rendered in the nginx configuration template. The PR checklist item for tests is also unchecked.
Per project policy, new features should include tests. Consider adding molecule/verify tests or template rendering tests that confirm:
- When
openwisp2_nginx_gzip_static: true, the rendered nginx config containsgzip_static on; - When
openwisp2_nginx_brotli_static: true, the rendered nginx config containsbrotli_static on; - When both are
false(default), neither directive appears in the config.
Code Review SummaryStatus: 8 Issues Found | Recommendation: Address before merge Fix these issues in Kilo Cloud Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code or context that cannot receive inline comments:
Files Reviewed (7 files)
|
…penwisp#599 Remove GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION settings from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst. Fixes openwisp#599
| apt: | ||
| name: libnginx-mod-http-gzip-static | ||
| when: openwisp2_nginx_install and openwisp2_nginx_gzip_static | ||
| ignore_errors: true # May not be available on all distributions |
There was a problem hiding this comment.
WARNING: ignore_errors: true silently swallows installation failures
If libnginx-mod-http-gzip-static fails to install (e.g., package not found), the task succeeds anyway. Then when nginx loads the config with gzip_static on; (rendered because openwisp2_nginx_gzip_static: true), nginx will fail with unknown directive "gzip_static". The user enabled the feature expecting it to work, but the failure is hidden.
Consider using failed_when: false with a register + subsequent warning task, or better yet, check if the module is actually loaded before rendering the directive in the template.
| apt: | ||
| name: libnginx-mod-brotli | ||
| when: openwisp2_nginx_install and openwisp2_nginx_brotli_static | ||
| ignore_errors: true # May not be available on all distributions |
There was a problem hiding this comment.
WARNING: ignore_errors: true silently swallows installation failures
Same issue as the gzip module task above: if libnginx-mod-brotli fails to install, the task succeeds silently, but nginx will then fail to start because brotli_static on; will be rendered in the config without the module being available. This creates a hard-to-debug failure mode where the user enabled the feature but gets a broken nginx.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
molecule/resources/verify.yml (1)
40-46:⚠️ Potential issue | 🟡 MinorDuplicate redis service checks - remove the incorrect one.
There are now two redis status checks: one checking
redis-server(line 41) and one checkingredis(line 45). Based on the role's handler and service task definitions, the correct systemd service name isredis, notredis-server(which is the APT package name).The
redis-servercheck on line 41 should be removed to avoid redundancy and potential confusion.🔧 Proposed fix
- - name: Check if redis-server is running - command: systemctl status redis-server - changed_when: false - - name: Check if redis is running command: systemctl status redis changed_when: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@molecule/resources/verify.yml` around lines 40 - 46, Remove the duplicate systemd status check for the APT package name: delete the task whose name is "Check if redis-server is running" (the command using "systemctl status redis-server") so only the correct service check "Check if redis is running" (command "systemctl status redis") remains; this aligns verify.yml with the role's service/task names and avoids redundancy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tasks/apt.yml`:
- Around line 43-53: The apt tasks "Install nginx gzip static module" and
"Install nginx brotli module" should stop silently swallowing failures; either
remove ignore_errors to fail fast or register each apt result (e.g., register:
gzip_mod_result and brotli_mod_result) and propagate those flags into the nginx
template rendering so gzip_static/brotli_static directives are only enabled when
installation succeeded; also make these tasks use the existing
retries/delay/register/until pattern used elsewhere to handle transient APT
errors (add retries/delay/until around the apt calls and register the final
result variable).
---
Outside diff comments:
In `@molecule/resources/verify.yml`:
- Around line 40-46: Remove the duplicate systemd status check for the APT
package name: delete the task whose name is "Check if redis-server is running"
(the command using "systemctl status redis-server") so only the correct service
check "Check if redis is running" (command "systemctl status redis") remains;
this aligns verify.yml with the role's service/task names and avoids redundancy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20b2b78c-fdf1-4535-a429-a0a59aa98005
📒 Files selected for processing (4)
defaults/main.ymlmolecule/resources/converge.ymlmolecule/resources/verify.ymltasks/apt.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build debian12
- GitHub Check: Build debian13
- GitHub Check: Build ubuntu2404
- GitHub Check: Build ubuntu2204
🔇 Additional comments (3)
defaults/main.yml (1)
163-170: LGTM!The new configuration variables are well-documented with clear warnings about module requirements and safe defaults (false). The placement as top-level variables (separate from
openwisp2_nginx_ssl_config) is correct and consistent with how they're referenced in the task conditions.molecule/resources/converge.yml (1)
15-18: LGTM!Good test coverage setup - both compression modules are enabled to validate the full feature path through the nginx configuration template and module installation tasks.
molecule/resources/verify.yml (1)
69-79: LGTM!Good verification coverage for the new compression features. The tests appropriately check:
- Both directives are present in the rendered nginx config
- The overall nginx configuration syntax is valid
This addresses the earlier review feedback about missing tests for the new configuration variables.
…penwisp#599 Remove GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION settings from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst. Fixes openwisp#599
| block: | ||
| # Test that enabled features appear in nginx config | ||
| - name: Check nginx config contains gzip_static and brotli_static directives | ||
| shell: "grep 'gzip_static on;' /etc/nginx/sites-enabled/openwisp2 && grep 'brotli_static on;' /etc/nginx/sites-enabled/openwisp2" |
There was a problem hiding this comment.
WARNING: Test hardcodes both directives unconditionally
This shell command checks for both gzip_static on; AND brotli_static on; in a single && chain, but the test will fail if only one of the two features is enabled. Since converge.yml enables both, this works for the current test scenario, but the test is fragile — if someone runs with only one feature enabled, the test will incorrectly fail.
Consider splitting into two separate tasks, each guarded by the corresponding variable:
- name: Check nginx config contains gzip_static directive
shell: "grep 'gzip_static on;' /etc/nginx/sites-enabled/openwisp2"
changed_when: false
when: openwisp2_nginx_gzip_static | default(false)
- name: Check nginx config contains brotli_static directive
shell: "grep 'brotli_static on;' /etc/nginx/sites-enabled/openwisp2"
changed_when: false
when: openwisp2_nginx_brotli_static | default(false)…penwisp#599 Remove GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION settings from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst. Fixes openwisp#599
|
|
||
| - name: Set nginx module availability facts | ||
| set_fact: | ||
| nginx_gzip_static_available: "{{ openwisp2_nginx_gzip_static and (gzip_static_module_install.skipped | default(false)) == false and gzip_static_module_install.failed | default(false) == false }}" |
There was a problem hiding this comment.
CRITICAL: Undefined variable error when openwisp2_nginx_gzip_static is false
When openwisp2_nginx_gzip_static is false, the Install nginx gzip static module task is skipped and gzip_static_module_install is never registered. Accessing gzip_static_module_install.skipped on an undefined variable will raise an Ansible error — the | default(false) filter only catches the final undefined value, not attribute access on an undefined dict.
Same issue applies to brotli_module_install on line 60.
Fix by using (gzip_static_module_install is defined and not gzip_static_module_install.skipped and not gzip_static_module_install.failed) or by adding a when guard:
- name: Set nginx module availability facts
set_fact:
nginx_gzip_static_available: >-
{{ openwisp2_nginx_gzip_static
and (gzip_static_module_install is defined)
and not (gzip_static_module_install.skipped | default(false))
and not (gzip_static_module_install.failed | default(false)) }}
nginx_brotli_static_available: >-
{{ openwisp2_nginx_brotli_static
and (brotli_module_install is defined)
and not (brotli_module_install.skipped | default(false))
and not (brotli_module_install.failed | default(false)) }}| register: gzip_static_check | ||
| changed_when: false | ||
|
|
||
| - name: Check if brotli_static directive is present (only when module available) |
There was a problem hiding this comment.
SUGGESTION: Trailing whitespace on this line may trigger linting warnings.
| - name: Check if brotli_static directive is present (only when module available) | |
| - name: Check if brotli_static directive is present (only when module available) |
| block: | ||
| # Check if modules were successfully installed and directives are present | ||
| - name: Check if gzip_static directive is present (only when module available) | ||
| shell: "grep 'gzip_static on;' /etc/nginx/sites-enabled/openwisp2 || echo 'DIRECTIVE_NOT_FOUND'" |
There was a problem hiding this comment.
WARNING: Test does not assert that the directive is present when the feature is enabled
The test uses || echo 'DIRECTIVE_NOT_FOUND' to avoid failure, and only logs the result via debug. This means the test will pass even if gzip_static on; is missing from the nginx config when openwisp2_nginx_gzip_static: true is set in converge.yml.
Since converge.yml sets openwisp2_nginx_gzip_static: true, the test should assert the directive is actually present:
- name: Check if gzip_static directive is present
shell: "grep 'gzip_static on;' /etc/nginx/sites-enabled/openwisp2"
register: gzip_static_check
changed_when: false
failed_when: gzip_static_check.rc != 0…penwisp#599 Remove GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION settings from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst. Fixes openwisp#599
… from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst. Fixes openwisp#599
Commit Message and Molecule FailuresHello @SAMurai-16, The CI failed due to two main issues:
Here's an example of the correct format:
|
… from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst. Fixes openwisp#599
Commit Message and Molecule FailuresHello @SAMurai-16, The CI failed due to two main issues:
|
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Remove GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION settings from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly. Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
This should be ready, will give it a try asap.
|
Proposed change log entry: |
With the previous configuration, the on-the-fly compression was still being used instead of the pre-compressed files.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@SAMurai-16 I made a number of improvements, feel free to see my changes. There were a number of problems:
- nginx logic was placed in the wrong file, it belongs to nginx.yml
- apt should be only concerned with installing system packages
- the brotli file was not being installed, the probability that some user would install it manually is low, so the brotli feature would be practically unused
- after testing on a VM with
straceI verified that nginx wasn't serving the precompressed gzip files, it was instead serving the on-the-fly gzipped, I had to change the config slightly to make it prefer the pre-compressed gzip files - lots of logic, especially the molecule tests, was too verbose
I appreciate the effort, but there's no point in submitting PRs if you don't verify on your end that the proposed changes really work so I ask you to refrain from doing this.
Please read our Anti AI Spam policy, with particular attention to the points which mention shifting the burder of testing on maintainers.
Thanks.
Remove GZIP_STATIC_COMPRESSION and BROTLI_STATIC_COMPRESSION settings from Django configuration as they are no longer valid with the new openwisp-utils package. Configure nginx to serve precompressed files by adding gzip_static and brotli_static directives to the SSL configuration. This improves performance by serving pre-compressed .gz and .br files directly when available, rather than compressing on-the-fly.
Updated templates/openwisp2/settings.py to remove obsolete compression settings, modified defaults/main.yml to add precompressed file serving configuration, and updated documentation in docs/user/role-variables.rst.
Fixes #599
Checklist
Reference to Existing Issue
Closes #599 .