fix: make Docker container ports configurable via env vars#694
fix: make Docker container ports configurable via env vars#694shivangtanwar wants to merge 3 commits intosrbhr:mainfrom
Conversation
start.sh hardcoded FRONTEND_PORT="3000" and BACKEND_PORT="8000" as static shell assignments, ignoring any environment variables passed to the container. Changed to use env var fallbacks so ports can be overridden at runtime (e.g. docker run -e FRONTEND_PORT=4000). Also updated docker-compose.yml to pass both port variables through and use FRONTEND_PORT in the container-side port mapping. Closes srbhr#610
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Review NotesWhat This PR DoesThis PR makes the frontend and backend ports configurable via:
Key Observations
Files Reviewed (3 files)
|
Add FRONTEND_PORT and BACKEND_PORT as build ARGs so EXPOSE and HEALTHCHECK are no longer hardcoded. Override with --build-arg when building with non-default ports.
|
Both fixed in 7df7c99. Added |
| # Health check on internal backend port only (independent of host port mapping). | ||
| HEALTHCHECK --interval=10s --timeout=10s --start-period=30s --retries=5 \ | ||
| CMD curl -f http://127.0.0.1:8000/api/v1/health || exit 1 | ||
| CMD curl -f http://127.0.0.1:${BACKEND_PORT}/api/v1/health || exit 1 |
There was a problem hiding this comment.
WARNING: HEALTHCHECK uses ${BACKEND_PORT} which is an ARG variable. Docker ARGs are only available at build time, not runtime.
Current behavior:
- If built with
--build-arg BACKEND_PORT=9000, healthcheck uses port 9000 ✓ - If run with
-e BACKEND_PORT=9000without rebuilding, healthcheck still uses port 8000 (build-time default) ✗
This is documented in the docker-compose.yml comment, but users might still be confused. Consider either:
- Using a shell wrapper script for the healthcheck that reads the env var at runtime
- Making the documentation more prominent about the rebuild requirement
|
Update: promoted ARGs to ENV in 9f2ad63 so HEALTHCHECK now resolves BACKEND_PORT at runtime too. Runtime |
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docker-compose.yml">
<violation number="1" location="docker-compose.yml:15">
P2: Compose comment instructs rebuilding with BACKEND_PORT for Next.js proxy rewrites, but rewrites are keyed off BACKEND_ORIGIN in next.config.ts. This guidance is incorrect and can leave rewrites pointing at the default backend origin/port after a port change.</violation>
</file>
<file name="Dockerfile">
<violation number="1" location="Dockerfile:114">
P2: Ports are declared with build-time ARG only, so runtime `-e FRONTEND_PORT/BACKEND_PORT` won’t update EXPOSE or HEALTHCHECK; health checks can hit the wrong port unless the image is rebuilt. Promote build args to ENV for runtime configurability.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Add ENV after ARG so HEALTHCHECK resolves BACKEND_PORT at runtime (allows override via docker run -e without rebuilding) - Fix docker-compose comment to reference BACKEND_ORIGIN instead of BACKEND_PORT for Next.js proxy rewrites
Summary
start.shhardcodedFRONTEND_PORT="3000"andBACKEND_PORT="8000"as static shell variables, so passing-e FRONTEND_PORT=4000todocker runhad no effect.${FRONTEND_PORT:-3000}), making ports truly configurable at runtime.docker-compose.ymlto pass both variables through and useFRONTEND_PORTin the container-side port mapping.Limitations
Changing
BACKEND_PORTalone isn't sufficient — the Next.js frontend hashttp://127.0.0.1:8000baked into its proxy rewrites at build time (next.config.tsandlib/api/client.ts). Users who need a different backend port must also rebuild the image with a matchingBACKEND_ORIGINbuild arg. This is noted in a comment indocker-compose.yml.Test plan
docker compose up— verify default behavior unchanged (ports 3000/8000)FRONTEND_PORT=4000 PORT=4000 docker compose up— verify frontend serves on 4000docker run -e FRONTEND_PORT=4000 -p 4000:4000 resume-matcher— verify port override worksCloses #610
Summary by cubic
Make Docker ports configurable via env vars at runtime and use build args for EXPOSE (defaults: frontend 3000, backend 8000). Healthcheck now reads BACKEND_PORT from env, so changing it doesn’t require a rebuild. Closes #610.
Bug Fixes
Migration
Written for commit 9f2ad63. Summary will update on new commits.