[fix] Make __openwisp_version__ dynamic to eliminate staleness #570#603
[fix] Make __openwisp_version__ dynamic to eliminate staleness #570#603CodingWithSaksham wants to merge 18 commits into
Conversation
…sp#570 Closes openwisp#570 The hardcoded __openwisp_version__ in openwisp/__init__.py was never updated during the release process, causing it to report an outdated version (e.g., 25.10.0 while the actual image was 25.10.2). This change makes the version dynamic by reading it from the OPENWISP_VERSION environment variable, which is set at Docker build time and baked into the image. This ensures __openwisp_version__ always reflects the actual image version without manual updates.
|
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:
📝 WalkthroughWalkthroughbuild.py gains commands to generate a version module (images/common/openwisp/_version.py) and to update RELEASE_VERSION in the Makefile. The Makefile calls python build.py generate-version $(OPENWISP_VERSION) and passes OPENWISP_VERSION as a --build-arg to the openwisp-base docker build; intermediate Docker build --target names were adjusted. images/common/openwisp/init.py now imports version as openwisp_version and exports it. .gitignore ignores the generated _version.py. Dockerfile edits are formatting-only. Sequence Diagram(s)sequenceDiagram
participant DevMachine
participant build_py
participant Makefile
participant DockerBuild
participant openwisp_base_Dockerfile
participant FinalImage
DevMachine->>build_py: python build.py generate-version <version>
build_py->>DevMachine: write images/common/openwisp/_version.py
DevMachine->>Makefile: run base-build (calls generator then docker build)
Makefile->>DockerBuild: docker build --build-arg OPENWISP_VERSION=<version>
DockerBuild->>openwisp_base_Dockerfile: provide ARG OPENWISP_VERSION
openwisp_base_Dockerfile->>FinalImage: build image (no behavioral changes)
FinalImage->>DevMachine: image contains code that imports generated _version.py
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental Review (Changes since 1555c0c)Files Changed:
Analysis:
Files Reviewed (5 files)
Reviewed by kimi-k2.5 · incremental review Reviewed by gpt-5.4-20260305 · 19,949 tokens |
|
I am not sure if unit tests should be added for this kind of bug. If we add unit tests we will have to update the version in the unit tests as well which defeats the point of this PR. |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/openwisp_base/Dockerfile`:
- Around line 51-53: The pip install RUN lines use unquoted build-arg expansions
which can break parsing and be unsafe; update each pip install invocation that
references OPENWISP_IPAM_SOURCE, OPENWISP_CONTROLLER_SOURCE,
OPENWISP_NOTIFICATION_SOURCE, OPENWISP_USERS_SOURCE and DJANGO_X509_SOURCE to
wrap the expansions in double quotes (e.g. "${OPENWISP_IPAM_SOURCE}") so the
shell treats them as single arguments and prevents word-splitting or injection;
apply the same quoting to every similar invocation in the Dockerfile (the pip
install lines around the shown RUN and the other occurrences noted).
- Line 142: Remove the hardcoded DJANGO_SECRET_KEY default from the Dockerfile
by eliminating the literal assignment "DJANGO_SECRET_KEY=DEFAULT_BAD_KEY" and
instead accept the secret via build/run-time environment variable or Docker
secret; update the Dockerfile's environment handling where DJANGO_SECRET_KEY is
referenced so it no longer sets a default value, and document that the runtime
must supply DJANGO_SECRET_KEY (e.g., via ENV passed at docker run,
docker-compose environment, or from a secrets manager) so the image does not
contain a shipped SECRET_KEY.
🪄 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: 3f51ddd5-e2b3-4b50-96e5-426178df75c6
📒 Files selected for processing (2)
images/common/openwisp/__init__.pyimages/openwisp_base/Dockerfile
📜 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). (2)
- GitHub Check: CI Build
- GitHub Check: Kilo Code Review
🧰 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:
images/common/openwisp/__init__.py
🪛 Checkov (3.2.529)
images/openwisp_base/Dockerfile
[low] 1-182: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 Trivy (0.69.3)
images/openwisp_base/Dockerfile
[error] 105-182: Secrets passed via build-args or envs or copied secret files
Possible exposure of secret env "DJANGO_SECRET_KEY" in ENV
Rule: DS-0031
(IaC/Dockerfile)
[error] 105-182: Secrets passed via build-args or envs or copied secret files
Possible exposure of secret env "EMAIL_HOST_PASSWORD" in ENV
Rule: DS-0031
(IaC/Dockerfile)
[error] 105-182: Secrets passed via build-args or envs or copied secret files
Possible exposure of secret env "SSH_PRIVATE_KEY_PATH" in ENV
Rule: DS-0031
(IaC/Dockerfile)
🔇 Additional comments (2)
images/common/openwisp/__init__.py (1)
3-8: LGTM!images/openwisp_base/Dockerfile (1)
9-23: LGTM!Also applies to: 72-73, 86-93, 97-101, 105-141, 143-182
Wrap build-arg variable expansions in double quotes to prevent shell word-splitting and malformed requirement parsing during pip install execution.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
This PR contains changes that are totally unrelated to the fix. Please clean it up and don't send PRs for review before having double checked and cleaned your work.
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_base/Dockerfile`:
- Line 88: The ARG OPENWISP_VERSION declared outside the openwisp_python stage
is unused because ARGs do not cross FROM boundaries; remove the stray ARG
declaration or move/re-declare ARG OPENWISP_VERSION inside the openwisp_python
stage so the build stage that needs OPENWISP_VERSION actually receives it—search
for the openwisp_python stage and the duplicate ARG OPENWISP_VERSION and keep
only the declaration inside the stage that consumes it (or delete the unused
one).
🪄 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: d2cf0d3d-20e0-4746-be56-0d538781fc96
📒 Files selected for processing (1)
images/openwisp_base/Dockerfile
📜 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
🪛 Checkov (3.2.529)
images/openwisp_base/Dockerfile
[low] 1-182: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 Trivy (0.69.3)
images/openwisp_base/Dockerfile
[error] 105-182: Secrets passed via build-args or envs or copied secret files
Possible exposure of secret env "DJANGO_SECRET_KEY" in ENV
Rule: DS-0031
(IaC/Dockerfile)
[error] 105-182: Secrets passed via build-args or envs or copied secret files
Possible exposure of secret env "EMAIL_HOST_PASSWORD" in ENV
Rule: DS-0031
(IaC/Dockerfile)
[error] 105-182: Secrets passed via build-args or envs or copied secret files
Possible exposure of secret env "SSH_PRIVATE_KEY_PATH" in ENV
Rule: DS-0031
(IaC/Dockerfile)
🔇 Additional comments (9)
images/openwisp_base/Dockerfile (9)
8-12: LGTM!
14-17: LGTM!
19-22: LGTM!
35-44: LGTM!
51-78: LGTM!
82-82: LGTM!Also applies to: 86-86
91-93: LGTM!
96-102: LGTM!
105-182: LGTM!
I am really sorry about the unrelated changes. I should have cleaned up the PR properly before sending it for review, and I will be more careful about that going forward. In this case, the unrelated changes were caused by the I will clean up the branch and update the PR with only the intended changes. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
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_base/Dockerfile`:
- Around line 88-89: The final build stage starting at the FROM system line uses
ENV OPENWISP_VERSION="${OPENWISP_VERSION}" but does not re-declare the build ARG
across the stage boundary; add an ARG OPENWISP_VERSION declaration immediately
after the FROM system line so that the build-time variable OPENWISP_VERSION is
available and correctly expands into ENV OPENWISP_VERSION in that stage.
🪄 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: 78d79a4d-4f40-4946-afb4-a0121167d21b
📒 Files selected for processing (1)
images/openwisp_base/Dockerfile
📜 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). (2)
- GitHub Check: CI Build
- GitHub Check: Kilo Code Review
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The approach is OK, but not complete yet: the base image build path still does not pass the version into the Dockerfile, so release images can still bake the default value. Left the important bits inline.
The base-build target was not passing --build-arg OPENWISP_VERSION to the final docker build command, causing openwisp-base to always be built with OPENWISP_VERSION=edge regardless of the RELEASE_VERSION set in the Makefile. This meant that `make release` could build and tag images as `openwisp-base:25.10.3` while the container internally reported version "edge", causing version mismatch and confusion in support scenarios.
|
@coderabbitai review |
✅ Actions performedReview 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/common/openwisp/__init__.py`:
- Around line 8-12: The file duplicates the environment lookup for
__openwisp_version__; remove the redundant assignment so there is a single
assignment to __openwisp_version__ via os.environ.get("OPENWISP_VERSION") placed
before the subsequent guard that checks if __openwisp_version__ is None or
"unknown", leaving __openwisp_installation_method__ untouched.
🪄 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: 97b192ed-71e9-465c-93e1-33481d0f574f
📒 Files selected for processing (3)
Makefileimages/common/openwisp/__init__.pyimages/openwisp_base/Dockerfile
📜 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:
images/common/openwisp/__init__.py
🧠 Learnings (1)
📚 Learning: 2026-02-17T16:59:39.027Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:39.027Z
Learning: In all Makefiles, prefer 'include' (not '-include') for critical config files like .env. If the file is missing, the build should fail explicitly rather than silently proceed with empty values. This ensures configurations are present and avoids dangerous silent failures. Consider adding a clear comment near the line and validation steps in CI to verify .env presence.
Applied to files:
Makefile
🪛 GitHub Actions: Merge Tests / 0_CI Build.txt
images/openwisp_base/Dockerfile
[warning] 104-104: SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ENV "EMAIL_HOST_PASSWORD") (line 104)
[warning] 104-104: SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ENV "DJANGO_SECRET_KEY") (line 104)
[warning] 104-104: SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ENV "SSH_PRIVATE_KEY_PATH") (line 104)
Makefile
[error] 71-71: Command failed: "make develop-pythontests && make stop". make: *** [Makefile:71: develop-pythontests] Error 1
🪛 GitHub Actions: Merge Tests / CI Build
Makefile
[error] 71-71: make develop-pythontests failed with exit code 1 after test_forgot_password error (FAILED (errors=1)).
🔇 Additional comments (2)
Makefile (1)
52-53: LGTM!images/openwisp_base/Dockerfile (1)
89-91: LGTM!
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thanks for addressing the first round, this is definitely moving in the right direction. I tested the build/runtime path locally and there is still one important correctness issue: the version is baked correctly, but compose can override it at runtime via .env, so __openwisp_version__ can still report the wrong value. I think the clean fix is to bake the version into an immutable file/module during the image build and read that from Python.
|
|
||
| __all__ = ["celery_app"] | ||
| __openwisp_version__ = "25.10.0" | ||
| __openwisp_version__ = os.environ.get("OPENWISP_VERSION") |
There was a problem hiding this comment.
This still reads the mutable runtime env, so compose can override the value baked into the image. I tested with make compose-build OPENWISP_VERSION=26.06a and then OPENWISP_VERSION=26.06a docker compose run --rm --no-deps --entrypoint python dashboard -c 'import openwisp, os; print(os.environ.get("OPENWISP_VERSION")); print(openwisp.__openwisp_version__)'. Expected: 26.06a / 26.06a. Actual: edge / edge, because .env contains OPENWISP_VERSION=edge and env_file wins at runtime. Please bake the build arg into an immutable file/module and read that instead.
There was a problem hiding this comment.
Do we also want to set OPENWISP_VERSION as an environment variable in the Dockerfile for the user, or is setting only __openwisp_version__ to the correct version sufficient?
| __openwisp_version__ = os.environ.get("OPENWISP_VERSION") | ||
| __openwisp_installation_method__ = "docker-openwisp" | ||
|
|
||
| if __openwisp_version__ is None or __openwisp_version__ == "unknown": |
There was a problem hiding this comment.
Raising from openwisp/__init__.py makes every package import fail if the env is missing or set unexpectedly. If the version is baked into a file/module at build time, this import-time guard should not be needed; if we keep a guard, I would put it in a narrower build/test verification path.
| @@ -48,8 +48,10 @@ base-build: | |||
| --target PYTHON ./images/ \ | |||
There was a problem hiding this comment.
While testing make compose-build OPENWISP_VERSION=26.06a, this stage printed ERROR: failed to solve: target stage "PYTHON" could not be found but the recipe continued and built the final images. Expected behavior: fail immediately on any failed Docker build step, or use a valid target and print no error. This may be pre-existing, but this PR now relies on this build path for version correctness.
There was a problem hiding this comment.
It appears
docker build --tag openwisp/openwisp-base:intermedia-system \
--file ./images/openwisp_base/Dockerfile \
--target SYSTEM ./images/; \was pointing to the wrong path as well, I will be adding a fix for both of these in this PR only.
I wonder how this issue went under the radar for so long 🤔
|
I am not sure why the CI keeps failing, whenever its re-run the CI passes. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I think the approach we had before was not bad, the problem is that it could be overridden with an env var which is wrong.
Any way, if you can't think of a better way to do this give me some more time to try this and come to a conclusion.
|
@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/common/openwisp/__init__.py`:
- Around line 12-28: The import-time RuntimeError comes from _read_version()
raising when _VERSION_FILE is missing or unreadable; change _read_version() so
that if os.path.exists(_VERSION_FILE) is False or an OSError occurs it returns a
sensible fallback string (e.g. "0+unknown" or "dev") instead of raising, and set
__openwisp_version__ to that fallback; update the error message to log a warning
if desired but ensure importing the module never raises by handling errors in
_read_version().
🪄 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: a49dd15c-bd6c-4e05-a458-d78469a7e3a4
📒 Files selected for processing (3)
Makefileimages/common/openwisp/__init__.pyimages/openwisp_base/Dockerfile
📜 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:
images/common/openwisp/__init__.py
🧠 Learnings (1)
📚 Learning: 2026-02-17T16:59:39.027Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:39.027Z
Learning: In all Makefiles, prefer 'include' (not '-include') for critical config files like .env. If the file is missing, the build should fail explicitly rather than silently proceed with empty values. This ensures configurations are present and avoids dangerous silent failures. Consider adding a clear comment near the line and validation steps in CI to verify .env presence.
Applied to files:
Makefile
🪛 GitHub Actions: Merge Tests / 0_CI Build.txt
Makefile
[error] 72-72: make develop-pythontests failed (Error 1). Test run did not succeed.
🔇 Additional comments (4)
Makefile (2)
45-45: LGTM!Also applies to: 48-48
50-54: LGTM!images/openwisp_base/Dockerfile (2)
90-91: LGTM!
98-99: LGTM!
Lets discuss the approach for this first, for now I have converted the PR to a draft PR, once we agree on a final approach I'll make it ready for review Multiple approaches do come to my mind
And honestly? The 2nd option seems more promising to me as I have seen libraries like numpy and pandas do the same, it is similar to our current approach but less flaky. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
images/common/openwisp/__init__.py (1)
3-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a safe fallback when generated
_version.pyis unavailable.Direct import from a generated module can break
import openwispin non-build contexts.🔧 Proposed fix
-from ._version import __version__ as __openwisp_version__ +try: + from ._version import __version__ as __openwisp_version__ +except ModuleNotFoundError: + __openwisp_version__ = "unknown"#!/bin/bash # Verify whether the generated module is tracked and whether __init__ has a fallback. git ls-files | rg -n '^images/common/openwisp/_version\.py$' || true sed -n '1,40p' images/common/openwisp/__init__.py🤖 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/common/openwisp/__init__.py` around lines 3 - 6, The direct import of __openwisp_version__ from the generated _version.py can fail in non-build contexts; update images/common/openwisp/__init__.py to import ._version within a try/except ImportError (or ModuleNotFoundError), set a safe fallback value for __openwisp_version__ (e.g., "0+unknown" or None) when the import fails, and keep celery_app import and __all__ unchanged so import openwisp never raises if _version.py is absent.images/openwisp_base/Dockerfile (1)
51-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQuote build-arg expansions in conditional
pip installcommands.These expansions are still unquoted, so build-arg values can be split into multiple arguments or interpreted unexpectedly by the shell.
🔧 Proposed fix
RUN if [ "$OPENWISP_IPAM_SOURCE" != "default" ] ; then \ - pip install --no-cache-dir --user --upgrade ${OPENWISP_IPAM_SOURCE}; \ + pip install --no-cache-dir --user --upgrade "${OPENWISP_IPAM_SOURCE}"; \ fi @@ RUN if [ "$OPENWISP_CONTROLLER_SOURCE" != "default" ] ; then \ - pip install --no-cache-dir --user --upgrade ${OPENWISP_CONTROLLER_SOURCE}; \ + pip install --no-cache-dir --user --upgrade "${OPENWISP_CONTROLLER_SOURCE}"; \ fi @@ RUN if [ "$OPENWISP_NOTIFICATION_SOURCE" != "default" ] ; then \ - pip install --no-cache-dir --user --upgrade ${OPENWISP_NOTIFICATION_SOURCE}; \ + pip install --no-cache-dir --user --upgrade "${OPENWISP_NOTIFICATION_SOURCE}"; \ fi @@ RUN if [ "$OPENWISP_USERS_SOURCE" != "default" ] ; then \ - pip install --no-cache-dir --user --upgrade --force-reinstall ${OPENWISP_USERS_SOURCE}; \ + pip install --no-cache-dir --user --upgrade --force-reinstall "${OPENWISP_USERS_SOURCE}"; \ fi @@ RUN if [ "$DJANGO_X509_SOURCE" != "default" ]; then \ - pip install --no-cache-dir --user --upgrade --force-reinstall ${DJANGO_X509_SOURCE}; \ + pip install --no-cache-dir --user --upgrade --force-reinstall "${DJANGO_X509_SOURCE}"; \ fi#!/bin/bash # Verify remaining unquoted variable expansions in pip install lines. rg -nP 'pip install[^\n]*\$\{(OPENWISP_IPAM_SOURCE|OPENWISP_CONTROLLER_SOURCE|OPENWISP_NOTIFICATION_SOURCE|OPENWISP_USERS_SOURCE|DJANGO_X509_SOURCE)\}' images/openwisp_base/Dockerfile🤖 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_base/Dockerfile` around lines 51 - 78, The pip install command invocations use unquoted build-arg expansions (e.g. OPENWISP_IPAM_SOURCE, OPENWISP_CONTROLLER_SOURCE, OPENWISP_NOTIFICATION_SOURCE, OPENWISP_USERS_SOURCE, OPENWISP_UTILS_SOURCE, DJANGO_X509_SOURCE) which can be split or misinterpreted by the shell; update each RUN ... pip install ... ${VAR} to quote the expansion (use "${VAR}") in the RUN blocks so each build-arg is passed as a single argument (preserve existing flags like --no-cache-dir, --user, --upgrade, --force-reinstall).
🤖 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 @.gitignore:
- Line 86: The .gitignore currently ignores any file named "_version.py"
(pattern "_version.py"), which is too broad; update the rule to scope it to the
generated OpenWISP file path by replacing the global pattern with the exact
relative path to the generated file (for example "openwisp/_version.py" or the
actual generated module path used in the project) so only that generated file is
ignored and unrelated "_version.py" files are not hidden.
In `@build.py`:
- Around line 47-52: The current branch where updated == content silently
returns and leaves stale RELEASE_VERSION; instead fail fast: when updated ==
content (i.e., no RELEASE_VERSION found in makefile_path), write an error to
stderr (including makefile_path and that RELEASE_VERSION was not found) and exit
with a non-zero status (e.g., call sys.exit(1) or raise a RuntimeError) so the
process fails CI/packaging; update the block that references updated, content,
and makefile_path to perform this error print+nonzero exit instead of returning.
- Around line 62-66: The generated _version.py writes raw version into the
Python source (variable content built in build.py), which can break if version
contains quotes/newlines; change the string injection to emit a properly escaped
Python string literal (use repr(version) or json.dumps(version)) when building
content so __version__ is assigned a safe quoted/escaped value, updating the
content construction in build.py where content is created and ensuring any
needed import (json) is added.
---
Duplicate comments:
In `@images/common/openwisp/__init__.py`:
- Around line 3-6: The direct import of __openwisp_version__ from the generated
_version.py can fail in non-build contexts; update
images/common/openwisp/__init__.py to import ._version within a try/except
ImportError (or ModuleNotFoundError), set a safe fallback value for
__openwisp_version__ (e.g., "0+unknown" or None) when the import fails, and keep
celery_app import and __all__ unchanged so import openwisp never raises if
_version.py is absent.
In `@images/openwisp_base/Dockerfile`:
- Around line 51-78: The pip install command invocations use unquoted build-arg
expansions (e.g. OPENWISP_IPAM_SOURCE, OPENWISP_CONTROLLER_SOURCE,
OPENWISP_NOTIFICATION_SOURCE, OPENWISP_USERS_SOURCE, OPENWISP_UTILS_SOURCE,
DJANGO_X509_SOURCE) which can be split or misinterpreted by the shell; update
each RUN ... pip install ... ${VAR} to quote the expansion (use "${VAR}") in the
RUN blocks so each build-arg is passed as a single argument (preserve existing
flags like --no-cache-dir, --user, --upgrade, --force-reinstall).
🪄 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: 83c027d8-3d83-4b9f-a43a-a24e0bfa61be
📒 Files selected for processing (5)
.gitignoreMakefilebuild.pyimages/common/openwisp/__init__.pyimages/openwisp_base/Dockerfile
📜 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:
images/common/openwisp/__init__.pybuild.py
🧠 Learnings (1)
📚 Learning: 2026-02-17T16:59:39.027Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:39.027Z
Learning: In all Makefiles, prefer 'include' (not '-include') for critical config files like .env. If the file is missing, the build should fail explicitly rather than silently proceed with empty values. This ensures configurations are present and avoids dangerous silent failures. Consider adding a clear comment near the line and validation steps in CI to verify .env presence.
Applied to files:
Makefile
🪛 Checkov (3.2.529)
images/openwisp_base/Dockerfile
[low] 1-179: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 Trivy (0.69.3)
images/openwisp_base/Dockerfile
[error] 102-179: Secrets passed via build-args or envs or copied secret files
Possible exposure of secret env "DJANGO_SECRET_KEY" in ENV
Rule: DS-0031
(IaC/Dockerfile)
[error] 102-179: Secrets passed via build-args or envs or copied secret files
Possible exposure of secret env "EMAIL_HOST_PASSWORD" in ENV
Rule: DS-0031
(IaC/Dockerfile)
[error] 102-179: Secrets passed via build-args or envs or copied secret files
Possible exposure of secret env "SSH_PRIVATE_KEY_PATH" in ENV
Rule: DS-0031
(IaC/Dockerfile)
🔇 Additional comments (1)
Makefile (1)
39-54: LGTM!
| content = ( | ||
| "# This file is auto-generated at Docker image build time.\n" | ||
| "# Do not edit manually.\n" | ||
| f'__version__ = "{version}"\n' | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify why raw interpolation is unsafe for Python source generation.
python - <<'PY'
import ast
cases = ["26.06a", '26.06a"rc1', "26.06a\nnext"]
for v in cases:
raw = f'__version__ = "{v}"\n'
safe = f"__version__ = {v!r}\n"
for label, src in [("raw", raw), ("safe", safe)]:
try:
ast.parse(src)
ok = True
except SyntaxError:
ok = False
print(f"{v!r:18} {label:4} -> parse_ok={ok}")
PYRepository: openwisp/docker-openwisp
Length of output: 316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect build.py around the referenced lines to confirm the exact interpolation.
nl -ba build.py | sed -n '45,90p'Repository: openwisp/docker-openwisp
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show build.py with line numbers without using `nl`
awk '{printf "%d\t%s\n", NR, $0}' build.py | sed -n '55,75p'Repository: openwisp/docker-openwisp
Length of output: 688
Escape version when generating images/common/openwisp/_version.py (build.py lines 62-66).
f'__version__ = "{version}"\n' writes raw version into Python source; if version contains quotes or newlines, the generated _version.py can become syntactically invalid and fail module parsing.
🔧 Proposed fix
content = (
"# This file is auto-generated at Docker image build time.\n"
"# Do not edit manually.\n"
- f'__version__ = "{version}"\n'
+ f"__version__ = {version!r}\n"
)🤖 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 `@build.py` around lines 62 - 66, The generated _version.py writes raw version
into the Python source (variable content built in build.py), which can break if
version contains quotes/newlines; change the string injection to emit a properly
escaped Python string literal (use repr(version) or json.dumps(version)) when
building content so __version__ is assigned a safe quoted/escaped value,
updating the content construction in build.py where content is created and
ensuring any needed import (json) is added.
There was a problem hiding this comment.
This becomes less important if we stop generating Python source during base-build, which I think we should. The better fix is to keep images/common/openwisp/_version.py tracked in the repo and update it through make update-version VERSION=.... If the helper writes that file, it should still use a safe Python literal such as {version!r} and fail if the update did not happen.
There was a problem hiding this comment.
@nemesifier, agreed — using !r ensures the version string is always a valid Python string literal regardless of its content. Here's the change:
- Apply the one-line fix in
build.pyto use{version!r}instead of"{version}"when generating_version.py
Failed to handle agent chat message. Please try again.
There was a problem hiding this comment.
This is stale after my updated comment above. Please do not keep generating this file during base-build. The requested approach is a tracked images/common/openwisp/_version.py, updated by a Makefile target and documented in the release/developer docs.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve the stale build-time generation comment from CodeRabbit on this PR.
# First find the comment ID to resolve it.
gh api graphql -f query='
{
repository(owner: "openwisp", name: "docker-openwisp") {
pullRequest(number: 603) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 5) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}'Length of output: 39519
@nemesifier, understood and apologies for the repeated stale reply. I'll drop the build-time generation framing entirely. To summarise the correct approach:
images/common/openwisp/_version.pyis a tracked repository file (not generated, not gitignored).- A single
make update-version VERSION=<x.y.z>Makefile target calls thebuild.pyhelper, which updates bothRELEASE_VERSIONinMakefileand__version__inimages/common/openwisp/_version.py. The helper should still use{version!r}when writing the Python file to produce a valid literal, and should fail hard if either file cannot be updated. - The Docker build only copies the tracked file into the image; it does not generate anything.
- The
make update-versiontarget is documented in the release/developer docs.
The generate_version_module function in build.py is therefore repurposed (or renamed) to be used only by that Makefile target at release time, not during base-build. The generate-version call in base-build (Makefile) should be removed.
✏️ Learnings added
Learnt from: nemesifier
Repo: openwisp/docker-openwisp PR: 603
File: build.py:61-65
Timestamp: 2026-05-29T15:14:20.076Z
Learning: In docker-openwisp (repo: openwisp/docker-openwisp), `images/common/openwisp/_version.py` must be a **tracked** repository file — never generated at build time or gitignored. The correct approach: a `make update-version VERSION=<x.y.z>` Makefile target invokes a `build.py` helper that updates both `RELEASE_VERSION` in `Makefile` and `__version__` in `images/common/openwisp/_version.py` in one step, using `{version!r}` for safe Python literal generation, and fails hard if either file is not found. The Docker build only copies the tracked file. The target must be documented in the release/developer docs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
… unit tests While running unit tests, there was a chance that the initial _version.py was not generated, so in the Makefile, we add a method to generate _version.py with a default 'edge' value so that import errors don't happen
| # 3. openwisp-monitoring: fping gdal-bin | ||
| # hadolint ignore=DL3008 | ||
| RUN apt-get update && \ | ||
| apt-get install --yes --no-install-recommends \ |
…_version__` in Makefile
Multiple Test Failures DetectedHello @CodingWithSaksham, The CI build failed due to multiple test failures. Here's a breakdown: 1. Test Failures:
These failures indicate issues with the application's functionality or the test suite's robustness. Please address these test failures to ensure the application behaves as expected and the tests are reliable. |
Test Failures in CIHello @CodingWithSaksham, The CI build failed due to the following test failures:
Note: The |
There was a problem hiding this comment.
I do not think the current generated ignored _version.py approach is the right solution. The version should be stored in a tracked file in the repo, updated by a Makefile target, and documented in the release/developer docs. Otherwise a clean checkout can fail to import openwisp, the source of truth is not reviewable, and the release output depends on a local generated artifact.
|
|
||
| # This is autogenerated by Docker when running | ||
| # make base-build or make runtests | ||
| from ._version import __version__ as __openwisp_version__ |
There was a problem hiding this comment.
After thinking this through, I do not think the generated ignored _version.py approach is acceptable. The version must be part of the repository, not an untracked build artifact. Otherwise a clean checkout cannot import openwisp, code review cannot see the source of truth, and the release process can silently build whatever happened to be generated locally.
Please change the approach to a tracked file, for example:
# images/common/openwisp/_version.py
__version__ = "25.10.0"Then __init__.py can import it directly. Add a Makefile target such as make update-version VERSION=25.10.3 which updates both RELEASE_VERSION in Makefile and images/common/openwisp/_version.py in the same command. Document that target in the developer or release docs. The Docker build should only copy the tracked file into the image, not generate an ignored module at build time.
| return keygen | ||
|
|
||
|
|
||
| def update_makefile_version(new_version: str): |
There was a problem hiding this comment.
This helper points in the right direction, but the implementation is incomplete. Updating only RELEASE_VERSION still leaves the Python version as a separate value, which is exactly the duplication this issue is about. It is also not wired to any Makefile target, so it is dead code in the current PR.
Please make this the single version update path instead: a Makefile target like make update-version VERSION=25.10.3 should call this helper, and the helper should update both RELEASE_VERSION in Makefile and the tracked images/common/openwisp/_version.py. It should fail if either file cannot be updated. Then document this target in the release/developer docs so maintainers know how to bump the version.
Test Failures in CIHello @CodingWithSaksham and @nemesifier, There are multiple test failures in the CI logs:
These failures are preventing the tests from completing successfully. Please address these issues and push a new commit. |
…dingWithSaksham/docker-openwisp into issues/570-fix-openwisp-version-stale
Commit Message and Test FailuresHello @CodingWithSaksham, The CI build failed due to two distinct issues:
Correct Format Example:
Please address the commit message format and investigate the test failures. |
2 participants
The hardcoded
__openwisp_version__inopenwisp/__init__.pywas never updated during the release process, causing it to report an outdated version (e.g., 25.10.0 while the actual image was 25.10.2).This change makes the version dynamic by reading it from the OPENWISP_VERSION environment variable, which is set at Docker build time and baked into the image. This ensures
__openwisp_version__always reflects the actual image version without manual updates.Checklist
Reference to Existing Issue
Closes #570 .
Description of Changes
images/openwisp_base/Dockerfile: Create a/opt/openwisp/OPENWISP_VERSIONfile that is immutable to any writes (after final build) and write build version into thisMakefile: Change docker target flags to correctly match build stage inimages/openwisp_base/Dockerfileand add a build arg in the last docker build to passOPENWISP_VERSIONinto the image.images/common/openwisp/__init__.py: Replace hardcoded "25.10.0" with a_read_version()function that reads/opt/openwisp/OPENWISP_VESRIONto read the version dynamically.Screenshot
N/A