refactor(docker): improve Dockerfile security, caching and build stru…#7325
refactor(docker): improve Dockerfile security, caching and build stru…#7325luic1ff wants to merge 1 commit intoopen-condo-software:mainfrom
Conversation
…cture Signed-off-by: luiciff <luiciff.employment@gmail.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThe Dockerfile architecture was restructured from a multi-stage design into a consolidated single-base approach with dedicated dependency and builder stages, including new runtime configurations, health checks, and user management improvements. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 4
🧹 Nitpick comments (1)
Dockerfile (1)
86-89: ConsiderCOPY --chownto avoid separatechown -Rstep.
chown -Ron a large/appdirectory adds build time and an extra layer. Using--chownon COPY is more efficient.Proposed optimization
-COPY --from=builder /app /app -COPY --from=python-deps /usr/local/lib/python3* /usr/local/lib/ - -RUN chown -R ${APP_USER}:${APP_USER} /app +COPY --from=builder --chown=${APP_USER}:${APP_USER} /app /app +COPY --from=python-deps /usr/local/lib/python3* /usr/local/lib/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 86 - 89, The Dockerfile currently does a separate RUN chown -R ${APP_USER}:${APP_USER} /app which adds time and a layer; instead modify the COPY instructions (COPY --from=builder /app /app and COPY --from=python-deps /usr/local/lib/python3* /usr/local/lib/) to use the --chown=${APP_USER}:${APP_USER} flag so files are owned correctly on copy and then remove the RUN chown -R ${APP_USER}:${APP_USER} /app step; ensure APP_USER is defined earlier in the Dockerfile and apply --chown to both COPY lines that provide files needing that ownership.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 37-38: The RUN step mounts /root/.cache/pip but also passes pip's
--no-cache-dir, which disables the benefit of the mount; update the Dockerfile
RUN command that invokes pip install (the RUN line installing -r
requirements.txt) to remove the --no-cache-dir flag so pip can use the mounted
cache (keep the --mount=type=cache,target=/root/.cache/pip as-is).
- Around line 3-7: The Dockerfile declares ARG PYTHON_VERSION but never uses it;
either remove the unused ARG or wire it into the image build (for example use
PYTHON_VERSION in the base image or package installation). If you choose to use
it, reference PYTHON_VERSION (the ARG) in the Dockerfile where Python is
installed (e.g., inject it into the FROM line or the apt/yum/pip install
command) so the variable is actually consumed, otherwise delete the ARG line to
avoid confusion with the other ARGs (REGISTRY, NODE_VERSION, APP_USER, APP_UID).
- Around line 96-104: The HEALTHCHECK in the Dockerfile references the
wrong-cased filename "healthcheck.js" which will fail on case-sensitive
filesystems; update the HEALTHCHECK command (the HEALTHCHECK line using CMD node
healthcheck.js || exit 1) to use the correct filename "healthCheck.js" so it
matches the actual module name (healthCheck.js) and prevents runtime failures.
- Around line 70-88: The Dockerfile references a missing requirements.txt and
includes an unused python-deps multi-stage and a COPY from that stage; either
remove the python-deps stage and the related COPY --from=python-deps line
(eliminating Python install lines and any ENV/PACKAGES tied to it) or, if Python
is required, add a valid requirements.txt and ensure the python-deps stage
installs those deps; locate the python-deps stage declaration and the COPY
--from=python-deps /usr/local/lib/python3* /usr/local/lib/ and update
accordingly to delete the dead Python stage and copy, or supply the missing
requirements.txt so the stage can succeed.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 86-89: The Dockerfile currently does a separate RUN chown -R
${APP_USER}:${APP_USER} /app which adds time and a layer; instead modify the
COPY instructions (COPY --from=builder /app /app and COPY --from=python-deps
/usr/local/lib/python3* /usr/local/lib/) to use the
--chown=${APP_USER}:${APP_USER} flag so files are owned correctly on copy and
then remove the RUN chown -R ${APP_USER}:${APP_USER} /app step; ensure APP_USER
is defined earlier in the Dockerfile and apply --chown to both COPY lines that
provide files needing that ownership.
| ARG REGISTRY=docker.io | ||
| ARG NODE_VERSION=24 | ||
| ARG PYTHON_VERSION=3.12 | ||
| ARG APP_USER=app | ||
| ARG APP_UID=10001 |
There was a problem hiding this comment.
PYTHON_VERSION ARG is defined but never used.
The PYTHON_VERSION ARG at line 5 is declared but not referenced anywhere in the Dockerfile. Either wire it into the base image or package installation, or remove it to avoid confusion.
Proposed fix: Remove unused ARG or use it
Option 1: Remove unused ARG
ARG REGISTRY=docker.io
ARG NODE_VERSION=24
-ARG PYTHON_VERSION=3.12
ARG APP_USER=app
ARG APP_UID=10001Option 2: Use it (e.g., in apt-get install)
- python3 \
- python3-pip \
- python3-venv \
+ python${PYTHON_VERSION} \
+ python${PYTHON_VERSION}-pip \
+ python${PYTHON_VERSION}-venv \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ARG REGISTRY=docker.io | |
| ARG NODE_VERSION=24 | |
| ARG PYTHON_VERSION=3.12 | |
| ARG APP_USER=app | |
| ARG APP_UID=10001 | |
| ARG REGISTRY=docker.io | |
| ARG NODE_VERSION=24 | |
| ARG APP_USER=app | |
| ARG APP_UID=10001 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 3 - 7, The Dockerfile declares ARG PYTHON_VERSION
but never uses it; either remove the unused ARG or wire it into the image build
(for example use PYTHON_VERSION in the base image or package installation). If
you choose to use it, reference PYTHON_VERSION (the ARG) in the Dockerfile where
Python is installed (e.g., inject it into the FROM line or the apt/yum/pip
install command) so the variable is actually consumed, otherwise delete the ARG
line to avoid confusion with the other ARGs (REGISTRY, NODE_VERSION, APP_USER,
APP_UID).
| RUN --mount=type=cache,target=/root/.cache/pip \ | ||
| pip install --no-cache-dir -r requirements.txt |
There was a problem hiding this comment.
Contradictory caching flags: --mount=type=cache with --no-cache-dir.
The cache mount at /root/.cache/pip is intended to persist pip's download cache across builds, but --no-cache-dir explicitly tells pip to skip its cache. Remove --no-cache-dir to actually benefit from the cache mount.
Proposed fix
RUN --mount=type=cache,target=/root/.cache/pip \
- pip install --no-cache-dir -r requirements.txt
+ pip install -r requirements.txt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN --mount=type=cache,target=/root/.cache/pip \ | |
| pip install --no-cache-dir -r requirements.txt | |
| RUN --mount=type=cache,target=/root/.cache/pip \ | |
| pip install -r requirements.txt |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 37 - 38, The RUN step mounts /root/.cache/pip but
also passes pip's --no-cache-dir, which disables the benefit of the mount;
update the Dockerfile RUN command that invokes pip install (the RUN line
installing -r requirements.txt) to remove the --no-cache-dir flag so pip can use
the mounted cache (keep the --mount=type=cache,target=/root/.cache/pip as-is).
| FROM ${REGISTRY}/node:${NODE_VERSION}-bookworm-slim AS runtime | ||
|
|
||
| ARG APP_USER | ||
| ARG APP_UID | ||
|
|
||
| ENV NODE_ENV=production \ | ||
| PYTHONUNBUFFERED=1 | ||
|
|
||
| RUN useradd --system \ | ||
| --uid ${APP_UID} \ | ||
| --create-home \ | ||
| --home-dir /app \ | ||
| ${APP_USER} | ||
|
|
||
| WORKDIR /app | ||
| COPY --from=builder --chown=app:app /app /app | ||
|
|
||
| COPY --from=builder /app /app | ||
| COPY --from=python-deps /usr/local/lib/python3* /usr/local/lib/ | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full Dockerfile structure
cd . && find . -name "Dockerfile" -type fRepository: open-condo-software/condo
Length of output: 81
🏁 Script executed:
# Look for werf.yaml to understand the application structure
find . -name "werf.yaml" -o -name "werf.yml" | head -5Repository: open-condo-software/condo
Length of output: 80
🏁 Script executed:
# Check the full Dockerfile context around the runtime stage
cat -n Dockerfile | head -100Repository: open-condo-software/condo
Length of output: 2733
🏁 Script executed:
# Check werf.yaml to understand what the application does
head -100 werf.yamlRepository: open-condo-software/condo
Length of output: 2273
🏁 Script executed:
# Check requirements.txt to see what Python packages are installed
cat requirements.txtRepository: open-condo-software/condo
Length of output: 117
🏁 Script executed:
# Search for any Python process spawning or execution in the application code
rg -i 'spawn|exec|child_process.*py|python' --type=js --type=ts -g '!node_modules' -C2 | head -80Repository: open-condo-software/condo
Length of output: 6516
🏁 Script executed:
# Search for requirements.txt throughout the entire repository
find . -name "requirements.txt" -type fRepository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
# Check for any build scripts or CI/CD configuration that references the Dockerfile
find . -name "*.yml" -o -name "*.yaml" -o -name "*.sh" | grep -E '(workflow|action|compose|script)' | head -20Repository: open-condo-software/condo
Length of output: 904
🏁 Script executed:
# Check if there are any references to the Dockerfile in the repository
rg -l "Dockerfile" --type=yaml --type=json --type=sh -g '!.git'Repository: open-condo-software/condo
Length of output: 87
🏁 Script executed:
# Check docker-compose.yml to see if it references the Dockerfile
cat docker-compose.yml | head -50Repository: open-condo-software/condo
Length of output: 1201
🏁 Script executed:
# Check deployment workflows to see which build system is used (werf vs Dockerfile)
rg -l "werf|Dockerfile" .github/workflows/ | head -5Repository: open-condo-software/condo
Length of output: 254
🏁 Script executed:
# Look at one of the production deployment workflows to understand the build process
head -80 .github/workflows/deploy_production.yamlRepository: open-condo-software/condo
Length of output: 3986
🏁 Script executed:
# Check git history to see if requirements.txt was recently removed or if it never existed
git log --all --full-history --oneline -- requirements.txt | head -20Repository: open-condo-software/condo
Length of output: 51
🏁 Script executed:
# Check if requirements.txt might be generated or created during build
rg "requirements\.txt" Dockerfile werf.yamlRepository: open-condo-software/condo
Length of output: 213
🏁 Script executed:
# Look for any Python dependencies or imports that might suggest the app needs Python
find apps packages -name "*.py" -type f 2>/dev/null | wc -lRepository: open-condo-software/condo
Length of output: 70
Dockerfile cannot build: missing requirements.txt file
The Dockerfile references requirements.txt on line 35, but this file does not exist in the repository. This will cause the python-deps stage to fail during build, breaking docker-compose.yml local development.
Additionally, the application contains zero Python files and is purely Node.js-based. The Python installation (lines 31-38, 87) is unnecessary dead code.
Either:
- Remove the entire
python-depsstage (lines 31-38) and the Python copy on line 87 since the application doesn't use Python - Add a
requirements.txtfile if Python support is actually required
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 70 - 88, The Dockerfile references a missing
requirements.txt and includes an unused python-deps multi-stage and a COPY from
that stage; either remove the python-deps stage and the related COPY
--from=python-deps line (eliminating Python install lines and any ENV/PACKAGES
tied to it) or, if Python is required, add a valid requirements.txt and ensure
the python-deps stage installs those deps; locate the python-deps stage
declaration and the COPY --from=python-deps /usr/local/lib/python3*
/usr/local/lib/ and update accordingly to delete the dead Python stage and copy,
or supply the missing requirements.txt so the stage can succeed.
| HEALTHCHECK --interval=30s --timeout=5s --retries=3 \ | ||
| CMD node healthcheck.js || exit 1 | ||
|
|
||
| ############################ | ||
| # Start | ||
| ############################ | ||
| EXPOSE 3000 | ||
|
|
||
| CMD ["node", "server.js"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if healthcheck.js and server.js exist in the repo
echo "=== Checking for healthcheck.js ==="
fd -t f 'healthcheck.js' | head -5
echo "=== Checking for server.js ==="
fd -t f 'server.js' | head -5
echo "=== Checking package.json for entry points ==="
fd -t f 'package.json' -d 1 -x jq -r '.scripts.start // .main // "not found"' {}Repository: open-condo-software/condo
Length of output: 502
🏁 Script executed:
#!/bin/bash
# Find the Dockerfile
echo "=== Finding Dockerfile ==="
fd -t f 'Dockerfile' | head -10
echo ""
echo "=== Checking app directory structure for the main app ==="
# Look for typical app directories
ls -la apps/ 2>/dev/null | head -20
echo ""
echo "=== Looking for server.js and healthcheck.js in apps ==="
fd -t f -e js 'server\.js|healthcheck\.js|healthCheck\.js' apps/ | head -20Repository: open-condo-software/condo
Length of output: 1528
🏁 Script executed:
#!/bin/bash
echo "=== Dockerfile content ==="
cat -n Dockerfile | head -110
echo ""
echo "=== Checking for server.js in root and common locations ==="
find . -maxdepth 3 -name "server.js" -type f 2>/dev/null | grep -v node_modules | head -20Repository: open-condo-software/condo
Length of output: 2944
🏁 Script executed:
#!/bin/bash
echo "=== Searching for all healthcheck variants ==="
fd -i 'healthcheck' | head -20
echo ""
echo "=== Checking if there's a healthcheck.js at root or in common build output ==="
ls -la healthcheck.js 2>/dev/null || echo "Not found at root"
echo ""
echo "=== Checking packages directory for healthcheck files ==="
find packages -name '*healthcheck*' -o -name '*healthCheck*' 2>/dev/null
echo ""
echo "=== Checking build scripts in package.json ==="
jq '.scripts | keys' package.json 2>/dev/null | head -30Repository: open-condo-software/condo
Length of output: 709
🏁 Script executed:
#!/bin/bash
echo "=== Contents of packages/keystone ==="
ls -la packages/keystone/ | head -30
echo ""
echo "=== Check if healthcheck.js is referenced anywhere in build/config ==="
rg 'healthcheck\.js' --type js --type json | head -20
echo ""
echo "=== Check app entry point expectations ==="
ls -la apps/condo/ | head -20Repository: open-condo-software/condo
Length of output: 3280
🏁 Script executed:
#!/bin/bash
echo "=== Contents of apps/condo/bin ==="
ls -la apps/condo/bin/
echo ""
echo "=== Check apps/condo/index.js first 50 lines ==="
head -50 apps/condo/index.js
echo ""
echo "=== Check if server.js or healthcheck.js are generated by looking at gitignore patterns ==="
cat .gitignore 2>/dev/null | grep -E 'server\.js|healthcheck\.js' || echo "No specific patterns found"Repository: open-condo-software/condo
Length of output: 5114
Fix case sensitivity: use healthCheck.js instead of healthcheck.js.
The Dockerfile references lowercase healthcheck.js, but the actual file is packages/keystone/healthCheck.js (camelCase). This will cause a runtime error on Linux due to case-sensitive filenames. Update line 97 to CMD node healthCheck.js || exit 1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 96 - 104, The HEALTHCHECK in the Dockerfile
references the wrong-cased filename "healthcheck.js" which will fail on
case-sensitive filesystems; update the HEALTHCHECK command (the HEALTHCHECK line
using CMD node healthcheck.js || exit 1) to use the correct filename
"healthCheck.js" so it matches the actual module name (healthCheck.js) and
prevents runtime failures.
|
1 participant
refactor(docker): improve Dockerfile structure and production readiness
Key improvements:
Summary by CodeRabbit