[feature] Configured nginx to serve precompressed files (gzip/brotli) #599#601
[feature] Configured nginx to serve precompressed files (gzip/brotli) #599#601SAMurai-16 wants to merge 19 commits intoopenwisp:masterfrom
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.
defaults/main.yml
Outdated
| - "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" |
docs/user/role-variables.rst
Outdated
| - "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}
defaults/main.yml
Outdated
| - "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
tasks/apt.yml
Outdated
| 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.
tasks/apt.yml
Outdated
| 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
molecule/resources/verify.yml
Outdated
| 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
tasks/apt.yml
Outdated
|
|
||
| - 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)) }}
molecule/resources/verify.yml
Outdated
| 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) |
molecule/resources/verify.yml
Outdated
| 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
tasks/apt.yml
Outdated
| when: openwisp2_nginx_install | ||
| changed_when: false | ||
|
|
||
| - name: Check if nginx brotli module is available |
There was a problem hiding this comment.
SUGGESTION: Trailing whitespace on this line may trigger yamllint warnings
| - name: Check if nginx brotli module is available | |
| - name: Check if nginx brotli module is available |
…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
Co-authored-by: kilo-code-bot[bot] <240665456+kilo-code-bot[bot]@users.noreply.github.com>
Co-authored-by: kilo-code-bot[bot] <240665456+kilo-code-bot[bot]@users.noreply.github.com>
…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
tasks/apt.yml
Outdated
|
|
||
| - name: Set nginx module availability facts | ||
| set_fact: | ||
| nginx_gzip_static_available: "{{ openwisp2_nginx_install and openwisp2_nginx_gzip_static and (gzip_static_check.stdout | default('not_available')) == 'available' }}" |
There was a problem hiding this comment.
CRITICAL: Undefined variable error when openwisp2_nginx_install is false
The set_fact task runs unconditionally, but gzip_static_check and brotli_check are only registered when openwisp2_nginx_install is true. When openwisp2_nginx_install is false, accessing gzip_static_check.stdout raises AnsibleUndefinedVariable — the | default('not_available') filter cannot catch attribute access on an undefined variable.
Fix by adding a when: openwisp2_nginx_install guard to the set_fact task, and providing a fallback for when the task is skipped:
- name: Set nginx module availability facts
set_fact:
nginx_gzip_static_available: "{{ openwisp2_nginx_gzip_static and (gzip_static_check.stdout | default('not_available')) == 'available' }}"
nginx_brotli_static_available: "{{ openwisp2_nginx_brotli_static and (brotli_check.stdout | default('not_available')) == 'available' }}"
when: openwisp2_nginx_install
- name: Set nginx module availability facts (nginx not installed)
set_fact:
nginx_gzip_static_available: false
nginx_brotli_static_available: false
when: not openwisp2_nginx_installAlternatively, use (gzip_static_check is defined and gzip_static_check.stdout == 'available') to safely handle the undefined case.
defaults/main.yml
Outdated
| - "font/opentype" | ||
| # Enable precompressed gzip files serving (requires nginx with gzip_static module) | ||
| # Note: The directive is only added if the module is already compiled into nginx | ||
| # If the module is not available, nginx will fail with "unknown directive" error |
There was a problem hiding this comment.
WARNING: Comment is inaccurate — nginx will NOT fail when the module is unavailable
The current implementation in tasks/apt.yml checks nginx -V and only sets nginx_gzip_static_available: true when the module is already compiled in. The template then conditionally omits the gzip_static on; directive when the module is absent. So nginx will not fail — the directive is silently skipped.
This comment misleads users into thinking they need the module pre-installed to avoid nginx failures, when in fact the role gracefully handles the absence.
| # If the module is not available, nginx will fail with "unknown directive" error | |
| # If the module is not available, the directive will be silently omitted from the nginx config |
defaults/main.yml
Outdated
| openwisp2_nginx_gzip_static: false | ||
| # Enable precompressed brotli files serving (requires nginx with ngx_brotli module) | ||
| # Note: The directive is only added if the module is already compiled into nginx | ||
| # If the module is not available, nginx will fail with "unknown directive" error |
There was a problem hiding this comment.
WARNING: Comment is inaccurate — nginx will NOT fail when the brotli module is unavailable
Same issue as the gzip comment above: the current implementation silently omits brotli_static on; when the ngx_brotli module is not compiled into nginx. The comment incorrectly states nginx will fail with an "unknown directive" error.
| # If the module is not available, nginx will fail with "unknown directive" error | |
| # If the module is not available, the directive will be silently omitted from the nginx config |
molecule/resources/verify.yml
Outdated
| register: nginx_gzip_module_check | ||
| changed_when: false | ||
|
|
||
| - name: Check if nginx brotli module is available |
There was a problem hiding this comment.
SUGGESTION: Trailing whitespace on this line may trigger yamllint warnings
| - name: Check if nginx brotli module is available | |
| - name: Check if nginx brotli module is available |
…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
defaults/main.yml
Outdated
| - "font/opentype" | ||
| # Precompressed gzip files serving - automatically enabled when nginx gzip_static/brotli_static module is available | ||
| # The system detects module availability and enables the feature automatically | ||
| # These variables are kept for backwards compatibility but no longer control the behavior |
There was a problem hiding this comment.
WARNING: Variables declared as "backwards compatibility" are actually dead code — they are never read by the role
The comment claims these variables are kept for backwards compatibility, but openwisp2_nginx_gzip_static and openwisp2_nginx_brotli_static are never referenced in tasks/apt.yml. The set_fact task that controls the feature only checks openwisp2_nginx_install and the nginx -V output:
nginx_gzip_static_available: "{{ openwisp2_nginx_install and (gzip_static_check.stdout | default('not_available')) == 'available' }}"This means:
- A user who sets
openwisp2_nginx_gzip_static: falseexpecting to disable the feature will find it still gets enabled if the module is available in their nginx build. - A user who sets
openwisp2_nginx_gzip_static: trueexpecting to enable the feature will find it has no effect if the module is absent.
The variables should either be wired into the set_fact logic (e.g. openwisp2_nginx_gzip_static and (gzip_static_check.stdout ...) == 'available') or removed entirely. Keeping them as no-ops while documenting them as user-facing configuration options is misleading.
…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
| when: openwisp2_nginx_install | ||
| changed_when: false | ||
|
|
||
| - name: Set nginx module availability facts |
There was a problem hiding this comment.
CRITICAL: set_fact runs unconditionally but gzip_static_check and brotli_check are only registered when openwisp2_nginx_install is true
Jinja2 does not short-circuit and — both sides of the expression are always evaluated. When openwisp2_nginx_install is false, gzip_static_check is an undefined variable, and accessing .stdout on it raises AnsibleUndefinedVariable. The | default('not_available') filter cannot rescue attribute access on an undefined dict.
Fix by adding a when: openwisp2_nginx_install guard and a fallback task:
- name: Set nginx module availability facts
set_fact:
nginx_gzip_static_available: >-
{{ (gzip_static_check.stdout | default('not_available')) == 'available' and
(openwisp2_nginx_gzip_static == true or openwisp2_nginx_gzip_static == 'auto') }}
nginx_brotli_static_available: >-
{{ (brotli_check.stdout | default('not_available')) == 'available' and
(openwisp2_nginx_brotli_static == true or openwisp2_nginx_brotli_static == 'auto') }}
when: openwisp2_nginx_install
- name: Set nginx module availability facts (nginx not installed)
set_fact:
nginx_gzip_static_available: false
nginx_brotli_static_available: false
when: not openwisp2_nginx_install
docs/user/role-variables.rst
Outdated
| # - false: explicitly disable even when module is available | ||
| openwisp2_nginx_gzip_static: auto | ||
| # Precompressed brotli files serving control | ||
| # - 'auto' (default): automatically enabled when nginx ngx_brotli module is detected |
There was a problem hiding this comment.
SUGGESTION: Trailing whitespace at end of line
| # - 'auto' (default): automatically enabled when nginx ngx_brotli module is detected | |
| # - 'auto' (default): automatically enabled when nginx ngx_brotli module is detected |
…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
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
It looks pretty good! I'll do some testing as soon as I can to confirm all works as expected and then merge. 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 .