[fix] Added DOCKER_TAG variable for version pinning to match intended release version#555
Conversation
…#554 this change introduces a DOCKER_TAG environment variable that allows users to pin specific image versions in .env file. Both 'docker compose pull' and 'make pull' now respect this variable, ensuring consistent version behavior across all deployment methods. Changes: - Add DOCKER_TAG=latest to .env file - Update all OpenWISP image tags in docker-compose.yml to use ${DOCKER_TAG:-latest} - Update Makefile to include .env and use DOCKER_TAG when retagging images Fixes openwisp#554
|
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:
📝 WalkthroughWalkthroughImage tag pinning was implemented across the repository. Two environment variables were added: IMAGE_OWNER and OPENWISP_VERSION (default "edge") in .env. The Makefile now includes .env, exposes OPENWISP_VERSION, IMAGE_OWNER and RELEASE_VERSION, and uses them in pull, build, publish, clean, and release targets. docker-compose.yml image references and Dockerfiles were changed to use OPENWISP_VERSION build args and tags. deploy/auto-install.sh was updated to set OPENWISP_VERSION via set_env and no longer writes/reads a VERSION file or passes TAG to make start. Documentation was updated to reflect these variables and targets. Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Env as .env
participant Make as Makefile
participant Registry as Docker Registry
participant Compose as docker-compose
Dev->>Env: add/set IMAGE_OWNER & OPENWISP_VERSION
Dev->>Make: run make pull / build / publish / start
Make->>Env: include .env, read IMAGE_OWNER, OPENWISP_VERSION, RELEASE_VERSION
Make->>Registry: pull/build and tag IMAGE_OWNER/<image>:OPENWISP_VERSION
Make->>Registry: push IMAGE_OWNER/<image>:OPENWISP_VERSION
Dev->>Compose: docker compose pull / up
Compose->>Env: resolve ${OPENWISP_VERSION:-edge}
Compose->>Registry: pull IMAGE_OWNER/<image>:OPENWISP_VERSION
sequenceDiagram
participant User as Installer
participant Script as deploy/auto-install.sh
participant Env as .env
participant Make as Makefile
User->>Script: run installer (may leave version blank)
Script->>User: prompt "OpenWISP Version (leave blank for latest stable release)"
Script->>Env: set_env OPENWISP_VERSION = provided_or_latest
Script->>Make: run make start
Make->>Env: include .env and use OPENWISP_VERSION for start
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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: 2
🤖 Fix all issues with AI agents
In @.env:
- Line 66: Move the DOCKER_TAG entry so it appears before EMAIL_DJANGO_DEFAULT
in the .env file to satisfy the dotenv-linter key ordering, and ensure the file
ends with a single trailing newline (add a blank line at EOF) so the linter no
longer reports a missing newline; update the DOCKER_TAG line (the literal key
"DOCKER_TAG") and verify EMAIL_DJANGO_DEFAULT remains unchanged.
In `@Makefile`:
- Line 26: The docker tag command can fail when DOCKER_TAG is empty; update the
recipe to use a fallback default for DOCKER_TAG (compose uses "latest") so the
tag is valid even if .env is missing. Locate the docker tag line in the Makefile
that references $${DOCKER_TAG} and replace it with a parameter expansion
fallback (e.g., use $${DOCKER_TAG:-latest}) so the command uses "latest" when
DOCKER_TAG is unset while keeping existing variables USER, image and TAG
unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.envMakefiledocker-compose.yml
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-06T11:56:48.600Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:85-85
Timestamp: 2026-01-06T11:56:48.600Z
Learning: In docker-openwisp projects, ensure CELERY_SERVICE_NETWORK_MODE is set to an empty string "" (which Docker Compose treats as unset/null). This allows containers to connect via the Compose default network with correct service name DNS resolution. Using "bridge" as the value disables service name resolution and breaks communication between celery, dashboard, postgres, and redis. Apply this guideline to docker-compose.yml files in the repository and any similar Compose files where CELERY services rely on service name DNS.
Applied to files:
docker-compose.yml
📚 Learning: 2026-01-06T11:55:13.672Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:85-85
Timestamp: 2026-01-06T11:55:13.672Z
Learning: In docker-compose.yml files, avoid leaving CELERY_SERVICE_NETWORK_MODE as an empty string; an empty value is ignored and Docker Compose falls back to default networking. Do not rely on setting CELERY_SERVICE_NETWORK_MODE to 'bridge' for celery services, as this will not affect their networking. If specific networking is required for celery services, define and attach explicit networks in the compose file and reference them on the celery services.
Applied to files:
docker-compose.yml
📚 Learning: 2026-01-06T11:28:41.789Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
Applied to files:
docker-compose.yml
🪛 dotenv-linter (4.0.0)
.env
[warning] 66-66: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 66-66: [UnorderedKey] The DOCKER_TAG key should go before the EMAIL_DJANGO_DEFAULT key
(UnorderedKey)
⏰ 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 comments (2)
Makefile (1)
4-6: LGTM — .env inclusion/export enables tag propagation.docker-compose.yml (1)
15-15: LGTM — parameterized tags meet pinning objective and keep defaults.Also applies to: 38-38, 58-58, 73-73, 88-88, 102-102, 116-116, 143-143, 159-159, 170-170
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Move DOCKER_TAG variable placement in .env file to satisfy alphabetical ordering requirements and add missing EOF newline. Update Makefile to use fallback value for DOCKER_TAG variable. Related to openwisp#554
atif09
force-pushed
the
issues/554-fix-docker-tag
branch
from
f60f251 to
a4f27a0
Compare
nemesifier
changed the title
github-project-automation
bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
github-project-automation
bot
moved this to In progress
in OpenWISP Priorities for next releases
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
github-project-automation
bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
Keeping DOCKER_TAG=latest avoids manual updating with each release, users will have to change this to the specific version by explicitly setting it to the desired version Related to openwisp#554
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.env (1)
5-12:⚠️ Potential issue | 🟡 MinorFix dotenv-linter key ordering for
DOCKER_TAG.
dotenv-linterexpectsDOCKER_TAGto appear beforeSSH_PRIVATE_KEY_PATH. Please reorder to satisfy lint rules.♻️ Proposed fix
DASHBOARD_DOMAIN=dashboard.openwisp.org API_DOMAIN=api.openwisp.org +# Image tag pinning +DOCKER_TAG=latest # SSH Credentials Configurations SSH_PRIVATE_KEY_PATH=/home/openwisp/.ssh/id_ed25519 SSH_PUBLIC_KEY_PATH=/home/openwisp/.ssh/id_ed25519.pub VPN_DOMAIN=openvpn.openwisp.org -DOCKER_TAG=latest EMAIL_DJANGO_DEFAULT=example@example.org
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.env
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-06T11:28:41.789Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
Applied to files:
.env
🪛 dotenv-linter (4.0.0)
.env
[warning] 11-11: [UnorderedKey] The DOCKER_TAG key should go before the SSH_PRIVATE_KEY_PATH key
(UnorderedKey)
⏰ 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
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
dotenv-linter expects DOCKER_TAG to appear before SSH_PRIVATE_KEY_PATH Related to openwisp#554
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
BTW we have an OPENWISP_VERSION var in the makefile.
However, I ask @pandafy to review this.
The variable in the makefile cannot serve as single source of truth if the someones uses docker to manage the images (e.g. docker compose pull) which is a common flow. |
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
This looks on the right track. But, we need to make sure that we have a single source of truth. IMO, we can use .env as single source of truth and make Makefile derive the version from there.
@nemesifier is there something I am missing here?
github-project-automation
bot
moved this from Reviewer approved
to In progress
in OpenWISP Priorities for next releases
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
@nemesifier don't merge this PR yet. i need to organise my thoughts and think about all possible situations.
I will report back on Monday.
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
The proposal is to introduce an OPENWISP_VERSION variable in the .env file, defaulting to edge, and update docker-compose.yml to reference images using openwisp/:${OPENWISP_VERSION}.
This approach allows the same configuration to support both local development builds and versioned, registry-based images, creating a clear distinction between development flexibility and production stability.
Effects on development flow
The development workflow remains essentially unchanged. Developers can continue using Makefile or docker compose commands directly. The default configuration will always use edge images. This makes working on new changes easy.
Effects on the release flow
edge images
edge images are published on every push to the master branch. This behavior is driven by the CI workflow and is unaffected by the proposed changes, so the edge release flow will remain exactly as it is today.
Versioned images (e.g. 25.10.2)
Currently, versioned images use the OPENWISP_VERSION value defined in the Makefile. With this proposal, OPENWISP_VERSION moves to .env and defaults to edge, where it becomes a runtime configuration rather than a release signal. As a result, it can no longer serve as the source of truth for release versioning.
To preserve a declarative and easy-to-maintain release process, we need a separate variable in the Makefile that explicitly defines the release version. This keeps the release version documented, version-controlled, and independent of runtime configuration.
While it would be possible to derive the version automatically from the latest Git tag, a declarative Makefile variable is preferred for clarity, predictability, and maintainability.
Effects on deployment
The auto-install script writes the user-selected version into the .env file during setup (defaulting to latest). As a result, deployments will use the latest stable images (e.g. 25.10). Alternatively, the user can enter the version number. This guarantees safer upgrades and predictable, reproducible installations without adding complexity to the development workflow.
If a user clones the docker-openwisp repository and spins up OpenWISP without using the auto-install script, then they will end up with the edge images (development version of OpenWISP).
There was a problem hiding this comment.
1. Does this branch solve the issue?
Yes, partially. The approach is correct:
- ✅ docker-compose.yml now uses
${OPENWISP_VERSION:-edge}instead of hardcoded:latest - ✅ auto-install.sh now persists
OPENWISP_VERSIONto.envfile instead of a separateVERSIONfile - ✅ Makefile
pulltarget now pulls the specific version tag
However, there's still a potential issue: If users don't set OPENWISP_VERSION in their .env, it defaults to edge (from the .env file). This is acceptable but could be confusing since edge is not a stable release.
2. Issues Found
A. Version Duplication (Critical)
The version is defined in three places with different purposes:
| File | Variable | Value | Purpose |
|---|---|---|---|
images/common/openwisp/__init__.py |
__openwisp_version__ |
"25.10.0" (hardcoded) |
Embedded in Docker image at build time |
.env |
OPENWISP_VERSION |
edge |
Used at runtime for pulling images |
Makefile |
RELEASE_VERSION |
25.10.0 |
Used for releases |
Problem: images/common/openwisp/__init__.py is never updated by any workflow. It contains stale version info.
Can we find a way to automatically put the right version in images/common/openwisp/__init__.py?
PS: I created a separate issue for this: #570
B. Makefile Variable Ordering Issue
USER = registry.gitlab.com/openwisp/docker-openwisp # Line 10
TAG = edge # Line 11
include .env # Line 3
OPENWISP_VERSION ?= edge # Line 13The include .env comes after USER and TAG definitions. If .env defined USER or TAG, they would be overridden. Currently it works, but it's fragile.
Recommendation: I think this is fine, we can just add a comment besides the include saying "The .env file can override variables in the Makefile, if needed".
C. pull Target Behavior Change
Before:
docker pull --quiet $(USER)/$${image}:$(TAG);
docker tag $(USER)/$${image}:$(TAG) openwisp/$${image}:latest;After:
docker pull --quiet $(USER)/$${image}:$(OPENWISP_VERSION);
docker tag $(USER)/$${image}:$(OPENWISP_VERSION) $(IMAGE_OWNER)/$${image}:$(OPENWISP_VERSION);Risk: If OPENWISP_VERSION doesn't exist on the registry (typo, deleted old tag), the command will fail. Previously it pulled :latest which always existed.
Question: would fallback to latest be ok @pandafy?
D. clean Target Missing New Images
The clean target now needs to remove additional tagged images:
$(IMAGE_OWNER)/openwisp-base:$(OPENWISP_VERSION) \
$(IMAGE_OWNER)/openwisp-nfs:$(OPENWISP_VERSION) \✅ This is already implemented (lines 82-83 in the diff).
E. Naming Confusion: RELEASE_VERSION vs OPENWISP_VERSION
OPENWISP_VERSION- comes from.env, used for pulling/pushingRELEASE_VERSION- hardcoded "25.10.0", used in release workflow
These should be more clearly documented or renamed to avoid confusion:
OPENWISP_VERSION→ could be calledIMAGE_TAG(the tag to pull/push)RELEASE_VERSION→ could stay as is (the version being released)
3. Security Considerations
- ✅ No security issues found
- ✅ No secrets hardcoded
- ✅ No new attack vectors introduced
4. Can we reuse the version from images/common/openwisp/__init__.py?
Answer: No, not directly, for these reasons:
-
It's not accessible at Makefile/runtime: The
__openwisp_version__is embedded in the built Docker image, not available at build time to the Makefile. -
It's already stale: The file shows
"25.10.0"but the latest release is"25.10.2". -
Wrong direction: The flow should be:
.envdefinesOPENWISP_VERSION→ used to pull images- Release workflow should update
images/common/openwisp/__init__.pyas part of the release
Better approach: Use a single source of truth - .env or a VERSION file - and automate the rest.
I created a separate issue: #570.
5. Summary & Recommendations
- Add fallback in
pulltarget if version doesn't exist - Rename variables for clarity (e.g.,
IMAGE_TAGinstead ofOPENWISP_VERSION) - Document the difference between
OPENWISP_VERSIONandRELEASE_VERSION
Added comments to clarify the purpose of RELEASE_VERSION and OPENWISP_VERSION and to note that .env can override Makefile variables if needed Related to openwisp#554
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 11: Add a .PHONY declaration for the Makefile target named "default" so
make will always run the compose-build recipe even if a file named "default"
exists; update the Makefile to include a .PHONY: default (you can also group
.PHONY: default compose-build) to mark the "default" target as phony.
- Around line 126-128: The release target in the Makefile relies on an implicit
OPENWISP_VERSION from the environment and can publish wrong images; modify the
release recipe so it explicitly passes OPENWISP_VERSION=$(RELEASE_VERSION) to
the publish calls (i.e., invoke make publish OPENWISP_VERSION=$(RELEASE_VERSION)
TAG=latest SKIP_TESTS=true and make publish OPENWISP_VERSION=$(RELEASE_VERSION)
TAG=$(RELEASE_VERSION) SKIP_BUILD=true SKIP_TESTS=true) so that publish (which
uses $(IMAGE_OWNER)/$${image}:$(OPENWISP_VERSION)) always uses the intended
RELEASE_VERSION rather than any preexisting .env value.
- Line 4: The comment on the `include .env` line is misleading: only variables
assigned with `?=` (e.g., `OPENWISP_VERSION`, `IMAGE_OWNER`) can be overridden
by .env, while variables set with plain `=` after the include (e.g.,
`RELEASE_VERSION`, `TAG`, `USER`, `SHELL`) will not be overridden; update the
comment to state this precisely and either move the `include .env` earlier or
change those assignments to `?=` if you want them overridable so
readers/operators know which variables can be pinned via .env.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Makefile
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: .env:7-9
Timestamp: 2026-02-17T17:08:12.144Z
Learning: In docker-openwisp, OPENWISP_VERSION=edge is intentionally set as the default in .env to maintain consistency with the existing TAG=edge default in the Makefile. Production users are expected to override OPENWISP_VERSION in their .env file to pin to a specific stable release tag.
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
📚 Learning: 2026-02-17T17:08:12.144Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: .env:7-9
Timestamp: 2026-02-17T17:08:12.144Z
Learning: In docker-openwisp, OPENWISP_VERSION=edge is intentionally set as the default in .env to maintain consistency with the existing TAG=edge default in the Makefile. Production users are expected to override OPENWISP_VERSION in their .env file to pin to a specific stable release tag.
Applied to files:
Makefile
📚 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
🪛 checkmake (0.2.2)
Makefile
[warning] 11-11: Target "default" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (1)
Makefile (1)
29-31: Pull and tag both correctly referenceOPENWISP_VERSION.The previously flagged issue (pulling
$(TAG)then relabelling asOPENWISP_VERSION) has been resolved. Bothdocker pullanddocker tagnow use the same$(OPENWISP_VERSION), so pinned version semantics are consistent.
…env comment openwisp#554 Explicitly pass OPENWISP_VERSION= in release target and clarified which variables can be overriden via .env Related to openwisp#554
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Makefile (1)
11-11: Already discussed:.PHONYfordefaulttarget.The checkmake warning about
defaultlacking a.PHONYdeclaration was previously raised and intentionally deferred by the author for consistency with the rest of the Makefile (which globally lacks.PHONY). No action needed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 11, Add a .PHONY declaration for the default target to silence the checkmake warning: update the Makefile to include a line like ".PHONY: default" (or add "default" to an existing .PHONY list) so the target named "default" is explicitly marked phony; ensure the symbol referenced is the default target in the Makefile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Makefile`:
- Line 11: Add a .PHONY declaration for the default target to silence the
checkmake warning: update the Makefile to include a line like ".PHONY: default"
(or add "default" to an existing .PHONY list) so the target named "default" is
explicitly marked phony; ensure the symbol referenced is the default target in
the Makefile.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Makefile
📜 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 (3)
📓 Common learnings
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: .env:7-9
Timestamp: 2026-02-17T17:08:12.144Z
Learning: In docker-openwisp, OPENWISP_VERSION=edge is intentionally set as the default in .env to maintain consistency with the existing TAG=edge default in the Makefile. Production users are expected to override OPENWISP_VERSION in their .env file to pin to a specific stable release tag.
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
📚 Learning: 2026-02-17T17:08:12.144Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: .env:7-9
Timestamp: 2026-02-17T17:08:12.144Z
Learning: In docker-openwisp, OPENWISP_VERSION=edge is intentionally set as the default in .env to maintain consistency with the existing TAG=edge default in the Makefile. Production users are expected to override OPENWISP_VERSION in their .env file to pin to a specific stable release tag.
Applied to files:
Makefile
📚 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
🪛 checkmake (0.2.2)
Makefile
[warning] 11-11: Target "default" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (3)
Makefile (3)
4-18: LGTM — variable declarations and.envintegration look correct.The
include .envis intentionally mandatory,OPENWISP_VERSION ?= edge/IMAGE_OWNER ?= openwispcorrectly use?=so.envtakes precedence, andRELEASE_VERSION/TAG/USERcorrectly use plain=to prevent accidental override. The inline comment on Line 4 accurately scopes what.envcan override.
29-31: LGTM — version pinning is correctly enforced in the pull target.Both the
docker pull(Line 29) anddocker tag(Line 30) now use$(OPENWISP_VERSION), which addresses the previously identified issue where$(TAG)was always pulled and then relabelled as a different version.
126-128: LGTM —releasetarget is now self-contained.Both invocations of
make publishnow explicitly passOPENWISP_VERSION=$(RELEASE_VERSION), ensuring the correct release images are sourced regardless of what.envhas set. This resolves the previously flagged implicit dependency.
nemesifier
changed the title
No, this is Infrastructure as Code. We shall clearly communicate this error to the user and provide rescue paths. |
There was a problem hiding this comment.
Great work @atif09! The PRs looks good overall.
I also tested the publish flows on my GitLab Container registry https://gitlab.com/pandafy/docker-openwisp/container_registry/10958718
I simulated the workflows executed when we merge a PR to master and when we release a new version. The changes behaved as expected.
@nemesifier While testing, I found that we will need to update the OPENWISP_VERSION variable in the .env file, otherwise it will stay as edge and override the OPENWISP_VERSION in the makefile.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code or outside the diff that cannot receive inline comments:
Issue AlignmentThis PR correctly addresses issue #554 by replacing hardcoded Policy Violations
Files Reviewed (4 files)
|
- Removing whitespace in auto-install.sh - Added docs for the new variables in settings.rst and instructions.rst - Updated Dockerfiles for api, dashboard, websocket - Changing the build process tag to use OPENWISP_VERSION Related to openwisp#554
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/auto-install.sh`:
- Line 62: The echo invocation uses unquoted variables ${GRN} and ${NON} which
can cause word-splitting (SC2086); update the echo line that builds the prompt
(the invocation referencing ${GRN} and ${NON}) to wrap those variable expansions
in double quotes (e.g., "${GRN}" and "${NON}") so the color variables are
treated as single tokens and avoid unintended splitting.
In `@docs/user/settings.rst`:
- Around line 40-41: The RST label ".._openwisp_version:" is malformed because
it lacks a space after the initial two dots; update the label to use correct RST
label syntax by inserting a space so it reads ".. _openwisp_version:" (ensure
the identifier "_openwisp_version" remains unchanged and no other characters are
altered).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eda8988a-26ce-4489-a846-331a40100362
📒 Files selected for processing (8)
Makefiledeploy/auto-install.shdocker-compose.ymldocs/developer/instructions.rstdocs/user/settings.rstimages/openwisp_api/Dockerfileimages/openwisp_dashboard/Dockerfileimages/openwisp_websocket/Dockerfile
📜 Review details
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: .env:7-9
Timestamp: 2026-02-17T17:08:12.144Z
Learning: In docker-openwisp, OPENWISP_VERSION=edge is intentionally set as the default in .env to maintain consistency with the existing TAG=edge default in the Makefile. Production users are expected to override OPENWISP_VERSION in their .env file to pin to a specific stable release tag.
📚 Learning: 2026-02-17T17:08:12.144Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: .env:7-9
Timestamp: 2026-02-17T17:08:12.144Z
Learning: In docker-openwisp, OPENWISP_VERSION=edge is intentionally set as the default in .env to maintain consistency with the existing TAG=edge default in the Makefile. Production users are expected to override OPENWISP_VERSION in their .env file to pin to a specific stable release tag.
Applied to files:
docker-compose.ymldeploy/auto-install.shdocs/user/settings.rstdocs/developer/instructions.rstMakefileimages/openwisp_websocket/Dockerfileimages/openwisp_api/Dockerfileimages/openwisp_dashboard/Dockerfile
📚 Learning: 2026-01-06T11:56:48.600Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:85-85
Timestamp: 2026-01-06T11:56:48.600Z
Learning: In docker-openwisp projects, ensure CELERY_SERVICE_NETWORK_MODE is set to an empty string "" (which Docker Compose treats as unset/null). This allows containers to connect via the Compose default network with correct service name DNS resolution. Using "bridge" as the value disables service name resolution and breaks communication between celery, dashboard, postgres, and redis. Apply this guideline to docker-compose.yml files in the repository and any similar Compose files where CELERY services rely on service name DNS.
Applied to files:
docker-compose.yml
📚 Learning: 2026-01-06T11:28:41.789Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
Applied to files:
docker-compose.yml
📚 Learning: 2026-02-17T16:59:43.808Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
Applied to files:
docker-compose.ymldeploy/auto-install.sh
📚 Learning: 2026-01-06T11:55:13.672Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:85-85
Timestamp: 2026-01-06T11:55:13.672Z
Learning: In docker-compose.yml files, avoid leaving CELERY_SERVICE_NETWORK_MODE as an empty string; an empty value is ignored and Docker Compose falls back to default networking. Do not rely on setting CELERY_SERVICE_NETWORK_MODE to 'bridge' for celery services, as this will not affect their networking. If specific networking is required for celery services, define and attach explicit networks in the compose file and reference them on the celery services.
Applied to files:
docker-compose.yml
📚 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
🪛 checkmake (0.2.2)
Makefile
[warning] 12-12: Target "default" should be declared PHONY.
(phonydeclared)
🪛 Shellcheck (0.11.0)
deploy/auto-install.sh
[info] 62-62: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 62-62: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (19)
images/openwisp_websocket/Dockerfile (1)
1-3: LGTM!The
ARG OPENWISP_VERSIONdeclaration beforeFROMcorrectly parameterizes the base image tag. This enables build-time version pinning and aligns with the same pattern used in the api and dashboard Dockerfiles.images/openwisp_dashboard/Dockerfile (1)
1-3: LGTM!Parameterized base image tag implementation is correct and consistent with the other Dockerfiles.
images/openwisp_api/Dockerfile (1)
1-3: LGTM!Consistent with the dashboard and websocket Dockerfiles. All three now properly support build-time version pinning.
docker-compose.yml (5)
14-22: LGTM for dashboard service image parameterization.The image tag and build argument are correctly set to use
${OPENWISP_VERSION:-edge}, ensuring version pinning works for both pulling pre-built images and building locally.
38-46: LGTM for api service.Consistent with dashboard service parameterization.
59-67: LGTM for websocket service.Consistent with dashboard and api service parameterization.
75-76: LGTM for celery services.All celery-related services correctly reference the dashboard image with the parameterized tag.
Also applies to: 90-91, 104-105
118-123: No action needed. Thenginx,freeradius,postfix, andopenvpnservices do not useopenwisp/openwisp-baseas a base image; they use external fixed-version base images (nginx:1.29.5-alpine,freeradius/freeradius-server:3.2.8-alpine,alpine:3.23, andlisenet/openvpn:2.6.17respectively). TheOPENWISP_VERSIONbuild argument is only needed for services that useopenwisp-base, which these services do not.> Likely an incorrect or invalid review comment.Makefile (4)
4-5: LGTM for mandatory.envinclusion.The
include .envdirective is intentionally mandatory to ensure critical configurations are present before running any targets. The comment accurately documents that only?=variables can be overridden.
16-19: LGTM for conditional variable defaults.Using
?=allows.envvalues to take precedence while providing sensible fallbacks (edgeandopenwisp) when variables aren't defined.
30-31: LGTM for pull target.The pull target now correctly uses
$(OPENWISP_VERSION)for both pulling and tagging, ensuring the pinned version is actually fetched rather than always pullingedge.
123-125: LGTM for release target.Explicitly passing
OPENWISP_VERSION=$(RELEASE_VERSION)ensures the release workflow publishes the correct images regardless of what's in.env.deploy/auto-install.sh (2)
131-131: LGTM for setup workflow usingset_envforOPENWISP_VERSION.Storing the version in
.envviaset_envis cleaner than maintaining a separateVERSIONfile. Themake startcommand will correctly readOPENWISP_VERSIONfrom.envsince the Makefile usesinclude .env.Also applies to: 182-182
195-195: LGTM for upgrade workflow.Consistent with the setup workflow—uses
set_envfor version persistence and simplifiedmake startinvocation.Also applies to: 205-205
docs/user/settings.rst (2)
42-51: LGTM forOPENWISP_VERSIONdocumentation.The documentation clearly explains the purpose, valid values (
edge,latest, semantic version), and default value. This aligns with the implementation.
1097-1103: LGTM forIMAGE_OWNERdocumentation.Clear explanation of the variable's purpose and appropriate placement in the Misc Services section.
docs/developer/instructions.rst (3)
117-120: LGTM for updated command documentation.The
startandpullcommands now correctly document theIMAGE_OWNERandOPENWISP_VERSIONparameters, reflecting the Makefile changes.
131-132: LGTM forpublishcommand documentation.Correctly updated to show the new parameter names.
136-143: LGTM for new target documentation.The new Makefile targets (
base-build,compose-build,develop-runtests,develop-pythontests) are well documented with clear descriptions.
- Fixed missing space in settings.rst - Quoted variables to prevent word splitting Related to openwisp#554
github-project-automation
bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
github-project-automation
bot
moved this from Reviewer approved
to Done
in OpenWISP Priorities for next releases
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
3 participants
this change introduces a DOCKER_TAG environment variable that allows users to pin specific image versions in .env file, both 'docker compose pull' and 'make pull' now work according to this variable, ensuring consistent version behavior across all deployment methods
Checklist
Reference to Existing Issue
Closes #554.
Description of Changes
Screenshot
version pinning test:
with
DOCKER_TAG=25.10.0in .env