[change] Added more env variables in nginx image#573
Conversation
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
|
@coderabbitai review |
|
@coderabbitai review! |
|
✅ Actions performedReview triggered.
|
|
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 environment-driven NGINX configuration: three new ENV vars (NGINX_WORKER_PROCESSES, NGINX_WORKER_CONNECTIONS, NGINX_EVENTS_BLOCK) in the Dockerfile; a templated nginx.template.conf that substitutes those vars and exposes worker_rlimit_nofile via ${NGINX_WORKER_RLIMIT_NOFILE}; and changes to images/common/init_command.sh that set NGINX_EVENTS_BLOCK, default NGINX_WORKER_RLIMIT_NOFILE to UNSET, run envsubst to generate /etc/nginx/nginx.conf from the template, then remove UNSET placeholders, any worker_rlimit_nofile directives, and empty lines. Sequence DiagramsequenceDiagram
participant Container as Container Startup
participant InitScript as init_command.sh
participant EnvSubst as envsubst
participant Cleanup as cleanup (sed)
participant NginxConf as /etc/nginx/nginx.conf
Container->>InitScript: execute init_command.sh
InitScript->>InitScript: set NGINX_EVENTS_BLOCK (printf)
InitScript->>InitScript: default NGINX_WORKER_RLIMIT_NOFILE=__UNSET__
InitScript->>EnvSubst: envsubst nginx.template.conf -> /etc/nginx/nginx.conf
EnvSubst->>NginxConf: write substituted nginx.conf
InitScript->>Cleanup: remove __UNSET__, worker_rlimit_nofile lines, empty lines
Cleanup->>NginxConf: update nginx.conf (cleaned)
NginxConf-->>Container: configuration ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/common/init_command.sh`:
- Around line 61-63: The script decodes NGINX_EVENTS_BLOCK after envsubst wrote
/etc/nginx/nginx.conf and emits a debug echo; move the printf "%b" decode so
NGINX_EVENTS_BLOCK is decoded before running envsubst (so the decoded multi-line
block is substituted into the generated nginx.conf), remove the debug echo line
that prints "$NGINX_EVENTS_BLOCK", and keep the existing sed cleanup (sed -i
'/__UNSET__/d; /^worker_rlimit_nofile *$/d; /^[ \t]*$/d' /etc/nginx/nginx.conf)
so only the decoded block is templated into the final file without logging raw
content.
In `@images/openwisp_nginx/nginx.template.conf`:
- Line 8: The template uses unsupported `${VAR:-default}` fallback so `envsubst`
leaves it unchanged; update the nginx.template.conf line to use plain
`${NGINX_WORKER_RLIMIT_NOFILE}` (or `$NGINX_WORKER_RLIMIT_NOFILE`) and ensure a
default is applied before envsubst by setting/exporting
NGINX_WORKER_RLIMIT_NOFILE in images/common/init_command.sh (e.g., assign
NGINX_WORKER_RLIMIT_NOFILE=${NGINX_WORKER_RLIMIT_NOFILE:-<desired_default>} or
export the value) so worker_rlimit_nofile is rendered correctly without relying
on envsubst to handle defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18600e99-e95c-4ae4-82be-174f70ef29c5
📒 Files selected for processing (3)
images/common/init_command.shimages/openwisp_nginx/Dockerfileimages/openwisp_nginx/nginx.template.conf
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-17T17:08:12.144Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: .env:7-9
Timestamp: 2026-02-17T17:08:12.144Z
Learning: In docker-openwisp, OPENWISP_VERSION=edge is intentionally set as the default in .env to maintain consistency with the existing TAG=edge default in the Makefile. Production users are expected to override OPENWISP_VERSION in their .env file to pin to a specific stable release tag.
Applied to files:
images/openwisp_nginx/Dockerfile
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/init_command.sh
🔇 Additional comments (2)
images/openwisp_nginx/Dockerfile (1)
45-47: Good defaulting for new nginx worker env vars.These defaults are sensible and keep backward-compatible behavior while enabling runtime overrides.
images/openwisp_nginx/nginx.template.conf (1)
15-16: Nice extensibility in theeventsblock.Parameterizing
worker_connectionsand allowing optional block injection here is a clean, scoped way to enable runtime tuning.
github-project-automation
bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
There was a problem hiding this comment.
♻️ Duplicate comments (1)
images/common/init_command.sh (1)
61-63:⚠️ Potential issue | 🔴 CriticalExport
NGINX_WORKER_RLIMIT_NOFILEbeforeenvsubst.Line 61 only assigns a shell-local variable. When
NGINX_WORKER_RLIMIT_NOFILEis not already exported, Line 62 rendersworker_rlimit_nofile ;, and Line 63 does not delete that line. The generatednginx.confis then invalid on the unset/default path.🐛 Proposed fix
- NGINX_WORKER_RLIMIT_NOFILE="${NGINX_WORKER_RLIMIT_NOFILE:-__UNSET__}" + export NGINX_WORKER_RLIMIT_NOFILE="${NGINX_WORKER_RLIMIT_NOFILE:-__UNSET__}"Run this to verify the behavior difference:
#!/bin/bash set -eu tmpl='worker_rlimit_nofile ${NGINX_WORKER_RLIMIT_NOFILE};\n' echo 'Current behavior (assigned, not exported):' sh -c 'NGINX_WORKER_RLIMIT_NOFILE="__UNSET__"; printf "%b" "'"$tmpl"'" | envsubst' echo echo 'Expected behavior (exported):' sh -c 'export NGINX_WORKER_RLIMIT_NOFILE="__UNSET__"; printf "%b" "'"$tmpl"'" | envsubst'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/common/init_command.sh` around lines 61 - 63, The envsubst call uses NGINX_WORKER_RLIMIT_NOFILE but the script only assigns it as a shell-local variable, so when not exported envsubst leaves an empty/invalid "worker_rlimit_nofile ;" in the generated file; change the assignment to export NGINX_WORKER_RLIMIT_NOFILE (i.e., export NGINX_WORKER_RLIMIT_NOFILE="${NGINX_WORKER_RLIMIT_NOFILE:-__UNSET__}") before calling envsubst in init_command.sh so envsubst sees the variable, and keep the existing sed cleanup to remove the __UNSET__ marker if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@images/common/init_command.sh`:
- Around line 61-63: The envsubst call uses NGINX_WORKER_RLIMIT_NOFILE but the
script only assigns it as a shell-local variable, so when not exported envsubst
leaves an empty/invalid "worker_rlimit_nofile ;" in the generated file; change
the assignment to export NGINX_WORKER_RLIMIT_NOFILE (i.e., export
NGINX_WORKER_RLIMIT_NOFILE="${NGINX_WORKER_RLIMIT_NOFILE:-__UNSET__}") before
calling envsubst in init_command.sh so envsubst sees the variable, and keep the
existing sed cleanup to remove the __UNSET__ marker if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4a61540-8203-4f94-b589-7828a2b3b122
📒 Files selected for processing (2)
images/common/init_command.shimages/openwisp_nginx/nginx.template.conf
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/init_command.sh
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
images/common/init_command.sh
Outdated
| NGINX_EVENTS_BLOCK=$(printf "%b" "${NGINX_EVENTS_BLOCK:-}") | ||
| NGINX_WORKER_RLIMIT_NOFILE="${NGINX_WORKER_RLIMIT_NOFILE:-__UNSET__}" | ||
| envsubst </etc/nginx/nginx.template.conf >/etc/nginx/nginx.conf | ||
| sed -i '/__UNSET__/d; /^worker_rlimit_nofile *$/d; /^[ \t]*$/d' /etc/nginx/nginx.conf |
There was a problem hiding this comment.
WARNING: \t in bracket expression not interpreted as tab by BusyBox sed
In Alpine Linux, sed is provided by BusyBox, which does not interpret \t as a tab character inside a bracket expression [ \t]. The pattern /^[ \t]*$/d will only match lines containing literal backslash and t characters, not actual tab characters. Lines consisting solely of tab characters will not be removed.
Use $'\t' expansion or a POSIX character class instead:
| sed -i '/__UNSET__/d; /^worker_rlimit_nofile *$/d; /^[ \t]*$/d' /etc/nginx/nginx.conf | |
| sed -i '/__UNSET__/d; /^worker_rlimit_nofile *$/d; /^[[:space:]]*$/d' /etc/nginx/nginx.conf |
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code or outside the diff that cannot receive inline comments:
Files Reviewed (3 files)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/common/init_command.sh`:
- Line 60: The decoded NGINX_EVENTS_BLOCK is currently assigned only as a shell
variable so envsubst can't see it; change the assignment so the value is
exported to the environment (e.g., export NGINX_EVENTS_BLOCK after or during the
printf assignment) so envsubst can substitute the decoded content into
nginx.conf; update the code around the NGINX_EVENTS_BLOCK assignment in
init_command.sh to ensure NGINX_EVENTS_BLOCK is an exported environment variable
visible to envsubst.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14a3300a-7c3a-4fa2-ab43-402e7e1367ee
📒 Files selected for processing (1)
images/common/init_command.sh
📜 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). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/init_command.sh
🔇 Additional comments (1)
images/common/init_command.sh (1)
61-63: LGTM!
- Line 61 correctly exports
NGINX_WORKER_RLIMIT_NOFILEwith a sentinel default for later cleanup.- Line 63 properly uses
[[:space:]]for POSIX-compliant blank line removal in BusyBoxsed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/common/init_command.sh`:
- Around line 60-61: The export using command substitution for
NGINX_EVENTS_BLOCK masks the exit status (SC2155); change to first assign the
expansion to the variable name NGINX_EVENTS_BLOCK (e.g.,
NGINX_EVENTS_BLOCK="$(printf "%b" "${NGINX_EVENTS_BLOCK:-}")") and then run
export NGINX_EVENTS_BLOCK in a separate statement so the printf exit code is not
hidden; apply the same pattern anywhere you see export combined with command
substitution (the current line exporting NGINX_EVENTS_BLOCK is the target).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88dbdff7-9c47-4dac-8f7f-216802564f72
📒 Files selected for processing (3)
images/common/init_command.shimages/openwisp_nginx/Dockerfileimages/openwisp_nginx/nginx.template.conf
📜 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). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-17T17:08:12.144Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: .env:7-9
Timestamp: 2026-02-17T17:08:12.144Z
Learning: In docker-openwisp, OPENWISP_VERSION=edge is intentionally set as the default in .env to maintain consistency with the existing TAG=edge default in the Makefile. Production users are expected to override OPENWISP_VERSION in their .env file to pin to a specific stable release tag.
Applied to files:
images/openwisp_nginx/Dockerfile
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/init_command.sh
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 60-60: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 60-60: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (4)
images/openwisp_nginx/Dockerfile (1)
45-47: LGTM!The new environment variables are well-structured with sensible defaults:
NGINX_WORKER_PROCESSES=1- appropriate single-worker default for containersNGINX_WORKER_CONNECTIONS=1024- standard default matching nginx defaultsNGINX_EVENTS_BLOCK=''- allows optional additional events configurationThe placement after
NGINX_CUSTOM_FILEgroups related nginx configuration variables logically.images/common/init_command.sh (1)
62-63: LGTM!The environment-driven nginx configuration generation is well implemented:
envsubstcorrectly generatesnginx.conffrom the template- The
sedcleanup properly removes__UNSET__placeholders, emptyworker_rlimit_nofiledirectives, and blank lines- Using
[[:space:]]instead of\tensures compatibility with BusyBox sed on Alpineimages/openwisp_nginx/nginx.template.conf (2)
7-8: LGTM!The template correctly uses plain
${VAR}syntax compatible with GNUenvsubst. Theworker_rlimit_nofiledirective with${NGINX_WORKER_RLIMIT_NOFILE}will be properly handled by the init script's sed cleanup when not set.
14-17: The sed pattern cleanup works correctly and doesn't need changes.The first sed pattern
/__UNSET__/dremoves the entire line containing__UNSET__(including the trailing semicolon). After envsubst withNGINX_WORKER_RLIMIT_NOFILE=__UNSET__, line 8 becomesworker_rlimit_nofile __UNSET__;, which is completely deleted by the first pattern, leaving no residual content. The second pattern/^worker_rlimit_nofile *$/dis redundant but harmless since the line no longer exists after the first pattern executes.> Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review and approve or summarize outstanding issues |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10254
Outstanding Issues1. # NGINX_WORKER_RLIMIT_NOFILE=65535 # Uncomment/set to enableOr document it clearly in the project docs. 2. Documentation not updated 3. Overly aggressive blank-line removal in sed -i '/__UNSET__/d; /^worker_rlimit_nofile *$/d; /^[[:space:]]*$/d' /etc/nginx/nginx.confThe third clause ( 4. Redundant What's Working Well
Items 1 and 2 are the blocking concerns; items 3 and 4 are minor improvements. Happy to approve once the documentation gap is addressed. |
| NGINX_CUSTOM_FILE=False \ | ||
| NGINX_WORKER_PROCESSES=1 \ | ||
| NGINX_WORKER_CONNECTIONS=1024 \ | ||
| NGINX_EVENTS_BLOCK='' \ |
There was a problem hiding this comment.
@pandafy shouldn't NGINX_WORKER_RLIMIT_NOFILE be listed here or not?
There was a problem hiding this comment.
It was not strictly required since we were providing the default value in the init command. For sake of completeness, I have added this.
2 participants
Checklist