Repository navigation
chore(docker): adopt hadolint 2.15.1 and fix the four findings it reports - #1700
Conversation
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request updates the Hadolint pre-commit image, changes runtime and development containers to numeric user identities, and makes the runtime health check explicitly invoke ChangesContainer and lint configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Follow-up to the digest pin in the previous commit, which restored the gate by freezing the linter at 2.14.0. Freezing is not the end state: 2.14.0 reports nothing only because it predates the rules, so staying there would have greened CI by un-seeing findings rather than by addressing them. Move the pin forward to 2.15.1, the version that surfaced them, and fix all four in the Dockerfiles. No `.hadolint.yaml` ignore entries and no inline `# hadolint ignore=` directives are used. DL3066, non-numeric user id (Dockerfile:149, Dockerfile.test:21, Dockerfile.dev:62). A name-form USER cannot be verified by Kubernetes `runAsNonRoot` and similar admission checks, which see only the numeric id. Dockerfile and Dockerfile.test already pinned their uids explicitly (1000 and 10001), so those are substitutions with no behaviour change. Dockerfile.dev created its user with `adduser -S`, which takes the first free system id and so varies with the base image; its uid/gid are now pinned to 1001 so the numeric USER has a value fixed at build time rather than discovered. Nothing outside Dockerfile.dev refers to `devuser`, so the changed id is contained. DL3025, JSON notation for CMD/ENTRYPOINT (Dockerfile:165). The flagged line is the HEALTHCHECK CMD, not the container ENTRYPOINT/CMD, which were already exec form. It reads `curl -f http://localhost:8080/health || exit 1`, where `||` is a shell operator: a naive exec-form list would pass `||` and `exit` to curl as literal arguments and the healthcheck would stop reporting unhealthy correctly. Written instead as `["/bin/sh", "-c", "..."]`, which is JSON notation and the same process tree the shell form produced, with the dependency on a shell declared rather than implied. Verified, with Docker available locally: - hadolint 2.15.1 against the three Dockerfiles: exit 0. - All three images build: Dockerfile, Dockerfile.dev, Dockerfile.test, exit 0. - Runtime identity is unchanged where it was already pinned: uid=1000(cudly) in the main image, uid=10001(e2e) in the test image, and uid=1001(devuser) in the dev image. - The healthcheck is recorded as ["CMD","/bin/sh","-c","curl -f http://localhost:8080/health || exit 1"] and exercised both directions: exit 1 with no server listening (so `||` is evaluated by the shell rather than handed to curl) and exit 0 on the short-circuit path. Closes #1695
bd4aad3 to
d7f6a6a
Compare
cristim
added
priority/p3
cristim
changed the title
![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 @.pre-commit-config.yaml:
- Around line 79-83: Update the Hadolint pin reference in the related CI comment
within the pre-commit configuration so it consistently states v2.15.1, matching
the executable hook configuration and the corresponding workflow documentation.
In `@Dockerfile.dev`:
- Around line 58-67: Update the Dockerfile user setup around devuser and USER
1001:1001 to preserve write access to the /app bind mount: accept DEV_UID and
DEV_GID build arguments and use them when creating the group/user, with
docker-compose.yml passing matching host IDs; alternatively configure a writable
named volume for .air.toml’s ./tmp/main output.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b360697-79ce-4346-ac7d-0ce4e251eea1
📒 Files selected for processing (4)
.pre-commit-config.yamlDockerfileDockerfile.devDockerfile.test
CodeRabbit found the note in .github/workflows/pre-commit.yml still claiming the hook is pinned to v2.14.0 after the pin moved to v2.15.1. That is the second time this comment has gone stale, and the earlier staleness is what hid the tag float behind the `rev:` pin in the first place. Rather than update the number and wait for it to rot again, remove the restated version and the `.pre-commit-config.yaml:81` line reference, which was also stale (the hook is at line 86 now). The comment points at the hook by `id:` instead, and names the digest entry as the single source of truth for the version. The remaining v2.14.0 mention further down is historical and accurate: it records what an earlier version of the comment wrongly claimed. Refs #1701
Review and decision record (merged)Follow-up to #1697, which unblocked CI by pinning hadolint to v2.14.0 by digest. That was correct as an unblock, but it left the four findings un-seen rather than fixed — 2.14.0 reports nothing only because it predates the rules. Green by freezing is the slow version of the same drift. This PR adopts 2.15.1 and fixes them for real. DL3025 — the one that could have broken the image. The flagged line is DL3066 — uid pinning. A pre-authorised exemption that was declined, and whyCodeRabbit raised a 🟠 Major on That justification does not survive checking. Run against the pinned base image, What could not be verified, stated rather than glossed: Docker Desktop on macOS remaps bind mounts to whatever uid the container runs as, so both the original and the new image write fine locally. That confirms no regression on macOS and proves nothing about Linux. Reproducing the failure needs a Linux host. The underlying problem is real and tracked as #1704, favouring a named volume for Air's output over The second thread was taken further than asked. A comment in Verification: |
…orts (#1700) * fix(docker): adopt hadolint 2.15.1 and fix the four findings it reports Follow-up to the digest pin in the previous commit, which restored the gate by freezing the linter at 2.14.0. Freezing is not the end state: 2.14.0 reports nothing only because it predates the rules, so staying there would have greened CI by un-seeing findings rather than by addressing them. Move the pin forward to 2.15.1, the version that surfaced them, and fix all four in the Dockerfiles. No `.hadolint.yaml` ignore entries and no inline `# hadolint ignore=` directives are used. DL3066, non-numeric user id (Dockerfile:149, Dockerfile.test:21, Dockerfile.dev:62). A name-form USER cannot be verified by Kubernetes `runAsNonRoot` and similar admission checks, which see only the numeric id. Dockerfile and Dockerfile.test already pinned their uids explicitly (1000 and 10001), so those are substitutions with no behaviour change. Dockerfile.dev created its user with `adduser -S`, which takes the first free system id and so varies with the base image; its uid/gid are now pinned to 1001 so the numeric USER has a value fixed at build time rather than discovered. Nothing outside Dockerfile.dev refers to `devuser`, so the changed id is contained. DL3025, JSON notation for CMD/ENTRYPOINT (Dockerfile:165). The flagged line is the HEALTHCHECK CMD, not the container ENTRYPOINT/CMD, which were already exec form. It reads `curl -f http://localhost:8080/health || exit 1`, where `||` is a shell operator: a naive exec-form list would pass `||` and `exit` to curl as literal arguments and the healthcheck would stop reporting unhealthy correctly. Written instead as `["/bin/sh", "-c", "..."]`, which is JSON notation and the same process tree the shell form produced, with the dependency on a shell declared rather than implied. Verified, with Docker available locally: - hadolint 2.15.1 against the three Dockerfiles: exit 0. - All three images build: Dockerfile, Dockerfile.dev, Dockerfile.test, exit 0. - Runtime identity is unchanged where it was already pinned: uid=1000(cudly) in the main image, uid=10001(e2e) in the test image, and uid=1001(devuser) in the dev image. - The healthcheck is recorded as ["CMD","/bin/sh","-c","curl -f http://localhost:8080/health || exit 1"] and exercised both directions: exit 1 with no server listening (so `||` is evaluated by the shell rather than handed to curl) and exit 0 on the short-circuit path. Closes #1695 * docs(ci): stop restating the hadolint version in the workflow comment CodeRabbit found the note in .github/workflows/pre-commit.yml still claiming the hook is pinned to v2.14.0 after the pin moved to v2.15.1. That is the second time this comment has gone stale, and the earlier staleness is what hid the tag float behind the `rev:` pin in the first place. Rather than update the number and wait for it to rot again, remove the restated version and the `.pre-commit-config.yaml:81` line reference, which was also stale (the hook is at line 86 now). The comment points at the hook by `id:` instead, and names the digest entry as the single source of truth for the version. The remaining v2.14.0 mention further down is historical and accurate: it records what an earlier version of the comment wrongly claimed. Refs #1701
1 participant
Closes #1701
What this is now
This PR was opened against #1695 before I discovered that #1697 had already landed the fix for it. #1695 is closed and
main'spre-commitis green as of218f3858e(12:53). The queue is unblocked, and nothing here is needed for that.The branch has been rebased onto current
main, and the duplicate pin commit dropped out automatically (git rebase: "dropping 0010d70... -- patch contents already upstream"). What remains is the halfmaindoes not have.What remains, and why it is worth landing
#1697 pinned the hadolint image to the v2.14.0 digest. That correctly fixes the root cause (the upstream
hadolint-dockerhook declaresentry: ghcr.io/hadolint/hadolint hadolintwith no image tag, sorev:pinned the hook definition while Docker resolved:latestevery run).But it leaves the four findings un-seen rather than fixed. 2.14.0 reports nothing only because it predates the rules. This PR moves the pin to 2.15.1 and fixes them, with no
.hadolint.yamlignore entries and no inline# hadolint ignore=directives.DL3066, non-numeric user id (
Dockerfile:149,Dockerfile.test:21,Dockerfile.dev:62). A name-formUSERcannot be verified by KubernetesrunAsNonRootand similar admission checks, which see only the numeric id.DockerfileandDockerfile.testalready pinned uids explicitly (1000, 10001), so those are substitutions with no behaviour change.Dockerfile.devusedadduser -S, which takes the first free system id and so varies with the base image; uid/gid are now pinned to 1001. Its uid value does change. Nothing outsideDockerfile.devrefers todevuser, so it is contained.DL3025, JSON notation (
Dockerfile:165). This is the one that could have broken the running image. The flagged line is the HEALTHCHECK CMD, not the containerENTRYPOINT/CMD(already exec form):CMD curl -f http://localhost:8080/health || exit 1||is a shell operator. Converting to["curl","-f","...","||","exit","1"]would hand||andexitto curl as literal arguments and the healthcheck would stop reporting unhealthy correctly. Written instead as["/bin/sh", "-c", "curl -f http://localhost:8080/health || exit 1"]— JSON notation, same process tree, shell dependency declared rather than implied.Verification
Docker was available, so this is built and exercised rather than reasoned about:
pre-commit run hadolint --all-files: exit 0.Dockerfile,Dockerfile.dev,Dockerfile.test, exit 0 each.uid=1000(cudly),uid=10001(e2e),uid=1001(devuser). The numeric form still resolves to the named user.["CMD","/bin/sh","-c","curl -f http://localhost:8080/health || exit 1"]and exercised both directions: exit 1 with no server listening (proving||is shell-evaluated, not passed to curl) and exit 0 on the short-circuit path.For the record, on the root cause
My first hypothesis on #1695 — that a pinned tag had been rebuilt underneath us — was wrong. The image was never pinned at all. Registry confirms
v2.14.0(sha256:27086352…) andv2.15.1(sha256:32dac941…, identical tolatest) are different images, and reproducing both directions locally gives exit 0 and exit 1 respectively.One unrelated failure, reported not fixed
pre-commit run --all-filesexits 1 locally on Terraform validate:Failed to install provider ... Error while installing hashicorp/azurerm v4.74.0: mkdir ...: file exists. That is a shared~/.terraform.d/plugin-cacherace between concurrent worktrees, not a config error. This diff touches zero.tffiles.If you would rather not take this
Closing is a reasonable call.
mainis green, and this is hygiene rather than a fix for anything currently broken. The argument for landing it is that staying on 2.14.0 means never picking up new hadolint rules, which is the slow version of the drift #1695 was about.Summary by CodeRabbit
Bug Fixes
Chores