[change] Avoid unnecessary CRL restarts when revokelist content is unchanged #589#610
[change] Avoid unnecessary CRL restarts when revokelist content is unchanged #589#6104f4d wants to merge 7 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR extracts OpenVPN-specific helper functions into a dedicated Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 63-71: Replace the fixed temporary filename used for CRL updates
to avoid races: in the code around local tmp_crl and calls to crl_download_to,
create a unique temp file (e.g., via mktemp or appending $$/timestamp) and
assign it to tmp_crl, use that variable for the download, comparison (cmp -s
"$tmp_crl" revoked.crl) and mv, and register a trap/cleanup to unlink tmp_crl on
failure/exit; update references to tmp_crl (the local variable and the
crl_download_to call) so all cleanup and the final move operate on the unique
temp file.
- Line 86: The cron line in openvpn_utils.sh uses a misspelled environment
variable (TOPLOGY_UPDATE_INTERVAL) causing an empty/invalid cron entry; update
the variable name to TOPLOGY_UPDATE_INTERVAL (matching the docs) in the echo
that writes the cron job and also fix the matching typo in the Dockerfile where
the variable is set so both use TOPLOGY_UPDATE_INTERVAL; ensure the cron string
still exports TOPOLOGY_UUID and TOPOLOGY_KEY as before and validate the
resulting cron syntax after the change.
- Around line 3-6: In get_redis_value(), replace the POSIX-incompatible echo -en
framing with a printf-based framing so the Redis GET request correctly includes
CRLF on ash/Alpine; use printf with a format string that inserts the key and
emits "\r\n" (e.g., printf 'GET %s\r\n' "$key") piped to nc redis 6379, keeping
the existing awk extraction unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9bc3b838-a66f-49ed-9fab-270ec767ba56
📒 Files selected for processing (6)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.sh
💤 Files with no reviewable changes (1)
- images/common/utils.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
🪛 Shellcheck (0.11.0)
images/openwisp_openvpn/openvpn.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/revokelist.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 5-5: In POSIX sh, echo flags are undefined.
(SC3037)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-38: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 35-36: Quote this to prevent word splitting.
(SC2046)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 37-37: Quote this to prevent word splitting.
(SC2046)
[warning] 40-48: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 47-47: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
[warning] 50-54: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 51-51: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 56-60: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 62-77: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 63-63: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 79-88: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 81-81: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 81-81: Quote this to prevent word splitting.
(SC2046)
[warning] 82-82: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 82-82: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (4)
images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/common/init_command.sh (1)
33-39: LGTM!images/openwisp_openvpn/openvpn.sh (1)
7-7: LGTM!images/openwisp_openvpn/revokelist.sh (1)
7-17: LGTM!
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
4944e57 to
4d2edf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 3-6: The get_redis_value function currently calls `nc redis 6379`
with no timeout; modify get_redis_value to run the netcat call with a bounded
timeout (e.g. using `timeout 5 nc redis 6379` or `nc -w 5 redis 6379`) and
detect a failure so the function returns an empty string (or non-zero) instead
of blocking; ensure you still pipe the successful response into `awk 'NR==2
{gsub(/\r/, ""); print}'` and handle the case where `timeout`/`nc` exits
non-zero by not printing garbage or hanging.
- Around line 34-48: The download/checksum functions openvpn_config_checksum and
openvpn_config_download don't fail fast or validate outputs; update them to
check curl exit statuses and non-empty outputs, aborting on failure: for
openvpn_config_checksum ensure the curl call succeeds and OFILE is non-empty
(exit or retry if not), and for openvpn_config_download verify each curl exit
code and that vpn.tar.gz and checksum files have expected size/content before
extracting; after tar xzf, verify the extraction succeeded and produced the
required .pem files and set strict permissions, failing (exit non-zero)
immediately on any validation error.
In `@tests/runtests.py`:
- Around line 465-469: After generating {test_dir}/revokelist.sh with the sed
substitutions in tests/runtests.py, add a quick post-transform assertion that
the output contains the expected stubbed lines (e.g. "cd {test_dir}/work" and
"source {test_dir}/openvpn_utils.sh" or the ":" replacement for "source
/utils.sh") and does not still contain the original patterns ("cd /" or "source
/utils.sh"). Locate the sed block that writes /revokelist.sh and, immediately
after writing the file, read {test_dir}/revokelist.sh and fail fast
(raise/assert with a clear message) if those expected replacements are not
present so the test deterministically fails when the sed patch stops matching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3122d9f-e85e-40f3-b293-e662291362fa
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.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
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 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
🪛 Shellcheck (0.11.0)
images/openwisp_openvpn/openvpn.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/revokelist.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-38: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 35-36: Quote this to prevent word splitting.
(SC2046)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 37-37: Quote this to prevent word splitting.
(SC2046)
[warning] 40-48: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 47-47: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
[warning] 50-54: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 51-51: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 56-60: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 62-82: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 63-63: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 84-93: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 86-86: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 86-86: Quote this to prevent word splitting.
(SC2046)
[warning] 87-87: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 87-87: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (5)
images/openwisp_openvpn/Dockerfile (1)
28-28: ⚡ Quick winThe typo fix is correct and consistently applied throughout the codebase.
The change from
TOPLOGY_UPDATE_INTERVALtoTOPOLOGY_UPDATE_INTERVALhas been properly integrated. No references to the old misspelling remain in the codebase, and the corrected variable name is consistently used in both the environment definition (Dockerfile), the consuming script (openvpn_utils.sh), and documentation (settings.rst).images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/common/init_command.sh (1)
33-39: LGTM!images/openwisp_openvpn/openvpn.sh (1)
7-7: LGTM!images/openwisp_openvpn/revokelist.sh (1)
7-17: LGTM!
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
2 times, most recently
from
86f364c to
f715d95
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
images/openwisp_openvpn/openvpn_utils.sh (2)
34-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on checksum/config download errors before applying artifacts.
These curl/tar/chmod steps currently continue on transient or malformed responses, which can trigger incorrect restart behavior.
Suggested fix
function openvpn_config_checksum { - export OFILE=$(curl --silent --insecure \ - ${API_INTERNAL}/controller/vpn/checksum/$UUID/?key=$KEY) - export NFILE=$(cat checksum) + OFILE="$(curl --silent --show-error --fail --insecure \ + "${API_INTERNAL}/controller/vpn/checksum/${UUID}/?key=${KEY}")" || return 1 + [ -n "$OFILE" ] || return 1 + NFILE="$(cat checksum 2>/dev/null)" || return 1 } function openvpn_config_download { - curl --silent --retry 10 --retry-delay 5 --retry-max-time 300\ - --insecure --output vpn.tar.gz \ - ${API_INTERNAL}/controller/vpn/download-config/$UUID/?key=$KEY - curl --silent --insecure --output checksum \ - ${API_INTERNAL}/controller/vpn/checksum/$UUID/?key=$KEY - tar xzf vpn.tar.gz - chmod 600 *.pem + curl --silent --show-error --fail --retry 10 --retry-delay 5 --retry-max-time 300 \ + --insecure --output vpn.tar.gz \ + "${API_INTERNAL}/controller/vpn/download-config/${UUID}/?key=${KEY}" || return 1 + curl --silent --show-error --fail --insecure --output checksum \ + "${API_INTERNAL}/controller/vpn/checksum/${UUID}/?key=${KEY}" || return 1 + tar xzf vpn.tar.gz || return 1 + chmod 600 -- ./*.pem || return 1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@images/openwisp_openvpn/openvpn_utils.sh` around lines 34 - 47, The openvpn_config_checksum and openvpn_config_download functions do not fail fast on curl/tar/chmod failures or on empty/malformed responses; update them so each external command is checked and the function exits non‑zero immediately on error: in openvpn_config_checksum verify the curl exit code and that OFILE is non-empty and well-formed before exporting, and in openvpn_config_download check the exit codes of both curl calls, validate the downloaded checksum file (NFILE) and compare it to OFILE (or compute a local checksum) before running tar xzf and chmod, and ensure tar and chmod errors are caught and cause an immediate exit; reference these functions (openvpn_config_checksum, openvpn_config_download) when adding these validations and early returns.
5-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to Redis reads to avoid hanging jobs.
nc redis 6379can block indefinitely and stall init/cron flows; use a bounded timeout and fail cleanly.Suggested fix
- printf 'GET %s\r\n' "$key" | nc redis 6379 | awk 'NR==2 {gsub(/\r/, ""); print}' + printf 'GET %s\r\n' "$key" | nc -w 5 redis 6379 | awk 'NR==2 {gsub(/\r/, ""); print}'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@images/openwisp_openvpn/openvpn_utils.sh` at line 5, The Redis read currently uses an unbounded netcat call ("printf ... | nc redis 6379 | awk ...") which can hang; wrap the nc call with a bounded timeout (e.g. use the timeout utility: "timeout 5s nc redis 6379" or netcat options like "nc -w 5 -q 1") and ensure the pipeline handles non-zero exit (treat timeout as a failure) so the script fails cleanly instead of hanging; update the line containing printf ... | nc redis 6379 | awk 'NR==2 {gsub(/\r/, ""); print}' to use the timeout-wrapped nc and propagate or handle its non-zero exit accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Line 91: The cron line writer currently echoes a crontab entry using
TOPOLOGY_UPDATE_INTERVAL without validation; add a guard before writing that
ensures TOPOLOGY_UPDATE_INTERVAL is non-empty and a positive integer (e.g.,
regex check like '^[0-9]+$'), and if invalid either skip creating the cron entry
and log a clear error/warning or fall back to a safe default interval; apply
this validation in the same scope that references TOPOLOGY_UPDATE_INTERVAL,
TOPOLOGY_UUID, TOPOLOGY_KEY and the /send-topology.sh cron line so the script
never writes an invalid cron entry.
In `@tests/runtests.py`:
- Around line 479-480: The test currently executes revokelist.sh with sh which
can break because the rewritten script uses the Bash-only builtin source; update
the invocation in tests/runtests.py so the command runs under bash (i.e.,
replace the use of "sh {test_dir}/revokelist.sh" with "bash
{test_dir}/revokelist.sh") so that the sourced openvpn_utils.sh and other Bash
semantics used by revokelist.sh execute correctly; ensure the environment prefix
(CRL_TEST_MODE and PATH adjustments) remains unchanged and still applies to the
bash invocation.
---
Duplicate comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 34-47: The openvpn_config_checksum and openvpn_config_download
functions do not fail fast on curl/tar/chmod failures or on empty/malformed
responses; update them so each external command is checked and the function
exits non‑zero immediately on error: in openvpn_config_checksum verify the curl
exit code and that OFILE is non-empty and well-formed before exporting, and in
openvpn_config_download check the exit codes of both curl calls, validate the
downloaded checksum file (NFILE) and compare it to OFILE (or compute a local
checksum) before running tar xzf and chmod, and ensure tar and chmod errors are
caught and cause an immediate exit; reference these functions
(openvpn_config_checksum, openvpn_config_download) when adding these validations
and early returns.
- Line 5: The Redis read currently uses an unbounded netcat call ("printf ... |
nc redis 6379 | awk ...") which can hang; wrap the nc call with a bounded
timeout (e.g. use the timeout utility: "timeout 5s nc redis 6379" or netcat
options like "nc -w 5 -q 1") and ensure the pipeline handles non-zero exit
(treat timeout as a failure) so the script fails cleanly instead of hanging;
update the line containing printf ... | nc redis 6379 | awk 'NR==2 {gsub(/\r/,
""); print}' to use the timeout-wrapped nc and propagate or handle its non-zero
exit accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6812144-d1fb-49ab-a7e1-3540b519df2e
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.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
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 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
🪛 Shellcheck (0.11.0)
images/openwisp_openvpn/openvpn.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/revokelist.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-38: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 35-36: Quote this to prevent word splitting.
(SC2046)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 36-36: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 37-37: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 37-37: Quote this to prevent word splitting.
(SC2046)
[warning] 40-48: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 43-43: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 45-45: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 47-47: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
[warning] 50-54: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 51-51: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 53-53: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 56-60: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 59-59: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 62-82: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 63-63: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 84-93: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 86-86: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 86-86: Quote this to prevent word splitting.
(SC2046)
[warning] 87-87: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 87-87: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (5)
images/openwisp_openvpn/Dockerfile (1)
28-28: ⚡ Quick winSpelling correction is correct and safe. The change from
TOPLOGY_UPDATE_INTERVALtoTOPOLOGY_UPDATE_INTERVALfixes a typo. No code references the misspelled name, and the correct spelling is already used inopenvpn_utils.sh(line 91) where the variable is actually referenced.images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/common/init_command.sh (1)
33-33: LGTM!Also applies to: 39-39
images/openwisp_openvpn/openvpn.sh (1)
7-7: LGTM!images/openwisp_openvpn/revokelist.sh (1)
7-7: LGTM!Also applies to: 10-17
4f4d
changed the title
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
f715d95 to
0340a4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 125-132: init_send_network_topology currently writes a cron entry
even when TOPOLOGY_UUID or TOPOLOGY_KEY are empty; change the logic so the cron
is only installed when both credentials are present: after loading/exporting
TOPOLOGY_UUID and TOPOLOGY_KEY (symbols TOPOLOGY_UUID, TOPOLOGY_KEY), test that
both are non-empty (e.g. [ -n "$TOPOLOGY_UUID" ] && [ -n "$TOPOLOGY_KEY" ])
before composing the crontab entry that uses TOPOLOGY_UPDATE_INTERVAL and
/send-topology.sh; if either is empty, skip creating the cron and avoid writing
an invalid job.
In `@images/openwisp_openvpn/revokelist.sh`:
- Line 7: The script revokelist.sh uses the Bash-only "source" builtin (e.g.
lines calling source /openvpn_utils.sh), which breaks when /bin/sh is dash/ash;
replace each use of "source" with the POSIX-compliant "." operator (e.g. ".
/openvpn_utils.sh") so the CRL refresh runs under /bin/sh; update every
occurrence of source in revokelist.sh (and any other scripts using source) to
the dot-sourcing form to ensure compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1819afc9-76f6-46f1-86cc-1a36a73b4324
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 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
🪛 Shellcheck (0.11.0)
images/openwisp_openvpn/openvpn.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/revokelist.sh
[warning] 7-7: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-51: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 36-36: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 53-81: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 54-54: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 55-55: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 83-87: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 84-84: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 89-93: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 95-115: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 96-96: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 117-133: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 126-126: Quote this to prevent word splitting.
(SC2046)
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 127-127: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (7)
images/openwisp_openvpn/Dockerfile (1)
28-28: No remaining references to the old variable name exist.The environment variable name correction from
TOPLOGY_UPDATE_INTERVALtoTOPOLOGY_UPDATE_INTERVALis valid and complete. Verification confirms all references inopenvpn_utils.shalready use the corrected spelling, and there are no lingering references to the old misspelled name in the codebase. The variable is also properly documented.images/openwisp_openvpn/openvpn_utils.sh (1)
3-123: LGTM!Also applies to: 133-134
images/common/init_command.sh (1)
33-39: LGTM!images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/openvpn.sh (1)
7-19: LGTM!images/openwisp_openvpn/revokelist.sh (1)
10-17: LGTM!tests/runtests.py (1)
7-7: LGTM!Also applies to: 315-320, 423-527, 530-546
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
0340a4c to
cf0c09c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 62-80: The mv of the downloaded checksum isn't checked so
openvpn_config_download can return success even if mv fails; update the mv
"$tmp_checksum" checksum operation to propagate failure (add || return 1) and
ensure the function uses the existing tmp_checksum/tmp_tar cleanup and trap
logic (symbols: openvpn_config_download, tmp_checksum, tmp_tar, mv
"$tmp_checksum" checksum, trap 'rm -f ...' EXIT HUP INT TERM) so any mv failure
causes the function to return 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53dff560-0432-4cfe-a4f0-868033fe248d
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.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
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 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
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-51: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 36-36: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 53-81: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 54-54: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 55-55: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 83-87: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 84-84: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 89-93: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 95-115: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 96-96: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 117-137: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 126-126: Quote this to prevent word splitting.
(SC2046)
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 127-127: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (9)
images/openwisp_openvpn/Dockerfile (1)
28-28: LGTM!images/openwisp_openvpn/revokelist.sh (1)
6-17: LGTM!tests/runtests.py (3)
7-7: LGTM!Also applies to: 315-322
423-531: LGTM!
534-551: LGTM!images/openwisp_openvpn/openvpn_utils.sh (1)
3-51: LGTM!Also applies to: 83-137
images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/openvpn.sh (1)
6-19: LGTM!images/common/init_command.sh (1)
33-33: LGTM!Also applies to: 39-39
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 106-109: The mv of "$tmp_crl" to revoked.crl is not checked so
failures still return success; modify the block that handles tmp_crl promotion
(the mv "$tmp_crl" revoked.crl call) to check mv's exit status and, on failure,
log an error (or echo to stderr), remove or preserve the tmp file as
appropriate, restore any trap state, and return a non-zero status instead of 0;
ensure you still clear the trap (trap - EXIT HUP INT TERM) only after a
successful mv to avoid reporting a false "changed" state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e696c45d-65b8-4356-9602-6c8c7fe5cc17
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.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
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 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
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-51: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 36-36: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 53-81: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 54-54: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 55-55: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 66-66: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 83-87: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 84-84: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 86-86: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 89-93: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 95-115: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 96-96: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 117-137: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 126-126: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 126-126: Quote this to prevent word splitting.
(SC2046)
[warning] 127-127: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 127-127: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (9)
images/openwisp_openvpn/Dockerfile (1)
28-28: LGTM!images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/openvpn.sh (1)
6-7: LGTM!Also applies to: 10-19
images/common/init_command.sh (1)
33-33: LGTM!Also applies to: 39-39
images/openwisp_openvpn/revokelist.sh (1)
6-17: LGTM!tests/runtests.py (4)
7-7: LGTM!
315-322: LGTM!
534-551: LGTM!
462-466: The conversion fromcrl_download_toreturn code 1 to exit status 2 is correctly implemented.The
crl_download_if_changedfunction (lines 101–103 inopenvpn_utils.sh) already converts the failure return code: whencrl_download_toreturns 1, the function immediately returns 2. This ensures the test stub'sreturn 1in fail mode properly triggers the error message assertion, as the calling code inrevokelist.shchecks forcrl_status -eq 2to emit the "Failed to download CRL" message.
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
@coderabbitai review |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
✅ Actions performedReview triggered.
|
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3). |
Commit Message and OpenVPN FailureHello @4f4d, Commit Message Failure: Fix: OpenVPN Container Failure: Fix: |
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
ad5c4bd to
1ae091b
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 126-130: crl_download_to currently does a single curl fetch which
causes crl_download_if_changed to fail on transient errors; modify
crl_download_to to perform multiple attempts with exponential backoff (e.g.,
configurable MAX_ATTEMPTS and initial DELAY, doubling delay each retry) while
still writing to the provided output_path and preserving curl flags (--silent
--show-error --fail --insecure --output). Keep the same function signature,
return non-zero on final failure so callers (crl_download_if_changed) detect
failure, and ensure the final successful attempt exits normally; include brief
comments and use the CA_UUID and DASHBOARD_INTERNAL variables as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34902685-9f02-4ef4-9502-ad27a3a67963
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 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
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-53: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 36-36: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 55-124: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 56-56: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 57-57: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 58-58: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 59-59: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 60-60: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 126-130: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 127-127: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 129-129: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 129-129: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 132-136: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 135-135: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 135-135: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 138-158: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 139-139: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 160-180: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 169-169: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 169-169: Quote this to prevent word splitting.
(SC2046)
[warning] 170-170: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 170-170: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (6)
images/common/init_command.sh (1)
33-33: LGTM!Also applies to: 39-39
images/openwisp_openvpn/openvpn.sh (1)
6-7: LGTM!Also applies to: 10-13, 16-19
images/openwisp_openvpn/revokelist.sh (1)
6-7: LGTM!Also applies to: 10-17
images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/Dockerfile (1)
28-28: LGTM!tests/runtests.py (1)
7-7: LGTM!Also applies to: 315-316, 318-320, 423-531, 532-678, 681-682, 687-688, 697-697
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 13-14: The separate call to `ip -6 route show default` before `if
[ $? = 0 ]; then` can fail under `set -e` and abort startup; instead, run the
command inside the conditional so failures don't trigger `set -e` (for example
replace the two-step pattern using `ip -6 route show default` and `if [ $? = 0
]; then` with a single conditional that executes the command inside the `if` and
redirects output to /dev/null). Update the usage of `ip -6 route show default`
and the `if [ $? = 0 ]; then` check accordingly so the script safely detects
IPv6 default routes without allowing `set -e` to cause an early exit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9fe58c9c-f81a-45f4-a7f8-8c22dd78cecf
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 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
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-20: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[style] 14-14: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
(SC2181)
[warning] 22-32: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 34-53: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 36-36: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 39-39: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 55-124: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 56-56: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 57-57: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 58-58: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 59-59: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 60-60: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 79-79: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 126-131: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 127-127: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 130-130: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 130-130: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 133-137: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 136-136: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 136-136: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 139-159: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 140-140: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 161-181: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 170-170: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 170-170: Quote this to prevent word splitting.
(SC2046)
[warning] 171-171: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 171-171: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (6)
images/common/init_command.sh (1)
33-33: LGTM!Also applies to: 39-39
images/openwisp_openvpn/openvpn.sh (1)
6-7: LGTM!Also applies to: 10-13, 16-19
images/openwisp_openvpn/revokelist.sh (1)
6-7: LGTM!Also applies to: 10-17
images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/Dockerfile (1)
28-28: LGTM!tests/runtests.py (1)
7-7: LGTM!Also applies to: 315-321, 423-678, 681-688, 697-697
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_openvpn/openvpn_utils.sh`:
- Around line 125-136: The crl_download function duplicates crl_download_to
(which already defaults output_path to revoked.crl); remove the redundant
crl_download function and update any callers to invoke crl_download_to (or keep
a tiny wrapper that forwards to crl_download_to) so there is a single
implementation. Specifically, delete the crl_download definition from the file
(or replace it with: crl_download() { crl_download_to; }) and ensure callers
referencing crl_download now call crl_download_to (or the new wrapper) to avoid
duplication.
- Around line 21-31: In openvpn_config, the unquoted command substitutions for
UUID, KEY and CA_UUID can be split if Redis values contain whitespace; update
the assignments that call get_redis_value to use quoted substitutions (i.e.,
assign UUID, KEY, CA_UUID from "$(get_redis_value ...)" ) so the results are
treated as single values and exported safely; touch the lines that export UUID,
KEY and CA_UUID in the function openvpn_config to apply this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 035d3067-477d-47fe-94b0-a7777efe7dae
📒 Files selected for processing (8)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/Dockerfileimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.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
📓 Path-based instructions (1)
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 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
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 8-19: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 21-31: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 27-27: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 27-27: Quote this to prevent word splitting.
(SC2046)
[warning] 28-28: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 28-28: Quote this to prevent word splitting.
(SC2046)
[warning] 29-29: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 29-29: Quote this to prevent word splitting.
(SC2046)
[warning] 33-52: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 34-34: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 38-38: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 38-38: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 38-38: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 54-123: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 55-55: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 56-56: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 57-57: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 58-58: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 59-59: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 74-74: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 74-74: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 74-74: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 78-78: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 78-78: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 78-78: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 125-130: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 126-126: In POSIX sh, 'local' is undefined.
(SC3043)
[info] 129-129: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 129-129: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 132-136: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[info] 135-135: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 135-135: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 138-158: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 139-139: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 160-180: 'function' keyword is non-standard. Use 'foo()' instead of 'function foo'.
(SC2113)
[warning] 169-169: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 169-169: Quote this to prevent word splitting.
(SC2046)
[warning] 170-170: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 170-170: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (15)
tests/runtests.py (4)
532-677: Test harness captures inner script exit code incorrectly on mismatch case.The test expects
exit_code=1for mismatch, butresult.exit_codecaptures the outersh -lcexit code, not the inneropenvpn.shexit code. Since the command ends after runningopenvpn.shwithout|| true, this should propagate correctly. However, if the inner script exits 1,set -euin the outer script should also cause exit 1.After re-reading: the outer script has
set -eubut thesh {test_dir}/openvpn.shis the last command, so its exit code propagates. This is correct.
7-7: LGTM!Also applies to: 315-322
423-530: LGTM!
679-698: LGTM!images/openwisp_openvpn/openvpn_utils.sh (6)
3-6: LGTM!
8-19: LGTM!
33-52: LGTM!
54-123: LGTM!
138-158: LGTM!
160-180: LGTM!images/common/init_command.sh (1)
33-39: LGTM!images/openwisp_openvpn/openvpn.sh (1)
6-21: LGTM!images/openwisp_openvpn/revokelist.sh (1)
6-17: LGTM!images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/openwisp_openvpn/Dockerfile (1)
28-28: LGTM!
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I think the main approach solves #589: comparing the downloaded CRL before restarting is the right direction, and moving the OpenVPN cron/helpers into the image makes this easier to maintain. I found one CRL validation gap that I would fix before merging.
| tmp_crl=$(mktemp /tmp/revoked.crl.XXXXXX) || return 2 | ||
| trap 'rm -f "$tmp_crl"' EXIT HUP INT TERM | ||
|
|
||
| if ! crl_download_to "$tmp_crl"; then |
There was a problem hiding this comment.
Can we treat an empty CRL as a failed download here? curl --fail only catches HTTP errors. If the dashboard returns 200 with an empty body, this function will promote that empty file at line 150 and restart OpenVPN, which can temporarily disable revocation checks. I would add a test -s "$tmp_crl" check before comparing/promoting, return 2 on empty, keep the existing revoked.crl, and add a small case to the regression test.
|
Hi @4f4d 👋, This is a friendly reminder that this pull request has had no activity for 7 days since changes were requested. We'd love to see this contribution merged! Please take a moment to:
If you're busy or need more time, no worries! Just leave a comment to let us know you're still working on it. Note: within 7 more days, the linked issue will be unassigned to allow other contributors to work on it. Thank you for your contribution! 🙏 |
Moved OpenVPN-specific helpers into the image directory, made CRL refresh restart OpenVPN only when the CRL changes, and moved the base OpenVPN cron entries into an image-local crontab. Closes openwisp#589
Preserve self-recovery when the local checksum is missing, verify downloaded OpenVPN config archives against the API checksum before applying them, and add regression coverage for mismatch and recovery cases. Related to openwisp#589
Use the same retry and backoff options for the temporary CRL download path that are already used for the direct CRL refresh path. Related to openwisp#589
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
2ea0394 to
810b3a9
Compare
Commit Message Style ViolationHello @4f4d, The CI has detected a violation in your commit message format. Specifically, the issue number is mentioned in the short description but not clearly indicated as closed or related. Fix: For example: |
Validate CRL downloads before replacing the live revoked.crl file. This keeps existing CRL contents when downloads fail or return empty responses, and keeps startup and refresh behavior aligned. Fixes openwisp#589
4f4d
force-pushed
the
issues/589-openvpn-crl-cron
branch
from
810b3a9 to
4a76bf8
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/common/init_command.sh`:
- Line 33: The script images/common/init_command.sh uses bash-only constructs:
replace every use of the bash builtin source (e.g., occurrences loading
utils.sh, docker-entrypoint.sh, openvpn_utils.sh) with the POSIX-compatible dot
operator (.) and convert all bash conditional expressions using [[ ... ]]
(notably the conditional around the block that begins near line 32 and the
conditionals around lines 93 and 104) to POSIX-compatible test/[ ... ] or case
constructs; update the expressions, string comparisons, and pattern checks
inside functions or conditional blocks so they use single-bracket [ ] or test,
replace =~ regex usage with case or grep if needed, and ensure variable
expansions are quoted to preserve behavior under /bin/sh (refer to
init_command.sh and the conditional blocks that call openvpn_utils.sh and the
surrounding functions to locate the exact places to change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 616697e0-4c50-420a-965c-952dbb476ddb
📒 Files selected for processing (7)
images/common/init_command.shimages/common/utils.shimages/openwisp_openvpn/openvpn.crontabimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/revokelist.shtests/runtests.py
💤 Files with no reviewable changes (1)
- images/common/utils.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
{images/**,customization/**,deploy/**}/*.sh
📄 CodeRabbit inference engine (AGENTS.md)
Avoid unnecessary blank lines inside functions or shell blocks
Files:
images/common/init_command.shimages/openwisp_openvpn/revokelist.shimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.sh
{**/Dockerfile*,**/*.sh}
📄 CodeRabbit inference engine (AGENTS.md)
Watch for exposed secrets, unsafe defaults, insecure permissions, unsafe shell expansion, path traversal, and accidental public ports in Docker configurations and shell scripts
Files:
images/common/init_command.shimages/openwisp_openvpn/revokelist.shimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/openvpn_utils.sh
**/*.{sh,py,yml,yaml,dockerfile,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Write comments only when they explain why code is shaped a certain way. Put comments before the relevant block instead of scattering them inside it
Files:
images/common/init_command.shimages/openwisp_openvpn/revokelist.shimages/openwisp_openvpn/openvpn.shtests/runtests.pyimages/openwisp_openvpn/openvpn_utils.sh
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
tests/runtests.py
🧠 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
🪛 Shellcheck (0.11.0)
images/common/init_command.sh
[warning] 33-33: In POSIX sh, 'source' in place of '.' is undefined.
(SC3046)
images/openwisp_openvpn/openvpn_utils.sh
[warning] 4-4: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 35-35: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 36-36: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 56-56: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 57-57: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 58-58: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 59-59: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 60-60: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 127-127: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 135-135: In POSIX sh, 'local' is undefined.
(SC3043)
[warning] 153-153: In POSIX sh, 'local' is undefined.
(SC3043)
🔇 Additional comments (20)
images/openwisp_openvpn/openvpn_utils.sh (9)
3-6: LGTM!
8-19: LGTM!
21-32: LGTM!
34-53: LGTM!
55-124: LGTM!
126-132: LGTM!
134-150: LGTM!
152-177: LGTM!
179-200: LGTM!images/openwisp_openvpn/openvpn.crontab (1)
1-2: LGTM!images/common/init_command.sh (1)
39-39: LGTM!images/openwisp_openvpn/openvpn.sh (2)
6-7: LGTM!
9-21: LGTM!images/openwisp_openvpn/revokelist.sh (2)
6-7: LGTM!
9-17: LGTM!tests/runtests.py (5)
9-9: LGTM!
354-363: LGTM!
466-586: LGTM!
587-733: LGTM!
734-753: LGTM!
Replace bash-specific source usage and conditionals in init_command.sh with POSIX-compatible syntax. This keeps the OpenVPN startup path compatible with the script's /bin/sh shebang. Related to openwisp#589
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Looks good, but I haven't tested it yet, I need to do one manual run to ensure it works as expected and then we can merge.
2 participants
Checklist
Reference to Existing Issue
Closes #589
Description of Changes
images/common/utils.shtoimages/openwisp_openvpn/openvpn_utils.shimages/openwisp_openvpn/openvpn.crontaband load them frominit_command.shrevokelist.shto download the CRL into a temporary file and restart OpenVPN only when the CRL actually changesTOPOLOGY_UPDATE_INTERVALbefore appending the optional topology cron entryLocal Validation
./run-qa-checkscompleted successfullymake compose-buildcompleted successfully, including the patched OpenVPN imageopenvpn.crontabis loaded correctlyrevoked.crland triggers the restart pathrevoked.crland triggers the restart pathrevoked.crland does not trigger a restart