Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
📝 WalkthroughWalkthroughThis PR hardens Docker build robustness across six Dockerfiles by enforcing timezone database ( ChangesDocker Build Hardening: Timezone and Dependency Installation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Workers/Resgrid.Workers.Console/Dockerfile (1)
44-65: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd file existence check in library extraction loop.
The
lddoutput parsing at line 64 doesn't verify that extracted paths are valid files before copying. Iflddoutputs virtual DSOs or the parsing yields invalid paths,cpwill fail with unclear errors. Other Dockerfiles in this codebase (e.g., Web.Tts line 85) use[ -f "$lib" ] && cpto filter out non-file entries.🛡️ Proposed defensive fix
&& wget -q https://github.com/wkhtmltopdf/packaging/releases/download/0.12.6.1-3/wkhtmltox_0.12.6.1-3.bookworm_amd64.deb \ && dpkg -i wkhtmltox_0.12.6.1-3.bookworm_amd64.deb || apt-get install -fy \ && rm -f wkhtmltox_0.12.6.1-3.bookworm_amd64.deb \ && mkdir -p /tmp/wkdeps \ - && ldd /usr/local/bin/wkhtmltopdf | awk '/=>/ {print $3}' | while read lib; do cp "$lib" /tmp/wkdeps/; done \ + && ldd /usr/local/bin/wkhtmltopdf | awk '/=>/ {print $3}' | while read lib; do [ -f "$lib" ] && cp "$lib" /tmp/wkdeps/; done \ && apt-get clean && rm -rf /var/lib/apt/lists/*🤖 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 `@Workers/Resgrid.Workers.Console/Dockerfile` around lines 44 - 65, The ldd parsing loop that copies runtime libraries for /usr/local/bin/wkhtmltopdf can attempt to cp non-file entries (virtual DSOs) and fail; update the loop that reads libs from ldd /usr/local/bin/wkhtmltopdf so it checks file existence before copying (e.g., test [ -f "$lib" ] && cp "$lib" /tmp/wkdeps/), ensuring only real files are copied into /tmp/wkdeps and avoiding cp errors during the Docker build.
🤖 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 `@Web/Resgrid.Web/Dockerfile`:
- Around line 27-29: The RUN instruction installing tzdata in the Dockerfile
(the line invoking DEBIAN_FRONTEND=noninteractive apt-get install -y --reinstall
--no-install-recommends tzdata and the subsequent test -f checks for zoneinfo
files) is missing the apt cache cleanup used in other Dockerfiles; update that
RUN chain to append && rm -rf /var/lib/apt/lists/* after the zoneinfo tests so
temporary apt lists are removed and the image layer size is minimized.
---
Outside diff comments:
In `@Workers/Resgrid.Workers.Console/Dockerfile`:
- Around line 44-65: The ldd parsing loop that copies runtime libraries for
/usr/local/bin/wkhtmltopdf can attempt to cp non-file entries (virtual DSOs) and
fail; update the loop that reads libs from ldd /usr/local/bin/wkhtmltopdf so it
checks file existence before copying (e.g., test [ -f "$lib" ] && cp "$lib"
/tmp/wkdeps/), ensuring only real files are copied into /tmp/wkdeps and avoiding
cp errors during the Docker build.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1eb9bc0c-b235-4dd4-94d7-f0ab38504243
📒 Files selected for processing (6)
Web/Resgrid.Web.Eventing/DockerfileWeb/Resgrid.Web.Mcp/DockerfileWeb/Resgrid.Web.Services/DockerfileWeb/Resgrid.Web.Tts/DockerfileWeb/Resgrid.Web/DockerfileWorkers/Resgrid.Workers.Console/Dockerfile
| RUN DEBIAN_FRONTEND=noninteractive apt-get install -y --reinstall --no-install-recommends tzdata \ | ||
| && test -f /usr/share/zoneinfo/America/New_York \ | ||
| && test -f /usr/share/zoneinfo/Asia/Kolkata |
There was a problem hiding this comment.
Add apt lists cleanup to match other Dockerfiles.
All other Dockerfiles in this PR append && rm -rf /var/lib/apt/lists/* after the zoneinfo file checks to minimize layer size. This one is missing that cleanup step.
🧹 Proposed fix
RUN DEBIAN_FRONTEND=noninteractive apt-get install -y --reinstall --no-install-recommends tzdata \
&& test -f /usr/share/zoneinfo/America/New_York \
- && test -f /usr/share/zoneinfo/Asia/Kolkata
+ && test -f /usr/share/zoneinfo/Asia/Kolkata \
+ && rm -rf /var/lib/apt/lists/*🤖 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 `@Web/Resgrid.Web/Dockerfile` around lines 27 - 29, The RUN instruction
installing tzdata in the Dockerfile (the line invoking
DEBIAN_FRONTEND=noninteractive apt-get install -y --reinstall
--no-install-recommends tzdata and the subsequent test -f checks for zoneinfo
files) is missing the apt cache cleanup used in other Dockerfiles; update that
RUN chain to append && rm -rf /var/lib/apt/lists/* after the zoneinfo tests so
temporary apt lists are removed and the image layer size is minimized.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
Release Notes