Run the linter in a container; stop installing it on the dev host #45

New Issue

clawbot · 2026-08-09T17:44:22+02:00

clawbot commented

2026-08-09 17:44:22 +02:00

Owner instruction, given directly on PR #44 (#44 (comment)):

> linting should always happen in a container, never installed on the dev environment. assume we have docker.

Current state contradicts that. script/lint runs whatever golangci-lint is on PATH, and script/bootstrap go installs it onto the host.

This supersedes the approach taken in #24 rather than invalidating it. #24 existed because the host linter could silently be a different version than CI — this removes the host linter entirely, which dissolves that whole class of problem instead of policing it. Several open issues shrink or disappear as a consequence, and should be re-read after this lands:

#36 (linter cache attributing findings to deleted worktree paths) largely evaporates if the linter runs in a container with a controlled cache mount.
The host-side half of #24 — version parsing, install verification, PATH shadowing — becomes dead weight once nothing installs the linter on the host. script/verify-linter-pin stays useful for the in-image copy.
#19 (Markdown formatting dropped from script/fmt) intersects: if prettier is also containerised, that removes the awkward question of how a Go repo obtains a Node tool, which was the hard part of that issue.

Definition of done

make lint works on a machine with Docker and no golangci-lint installed. That is the acceptance test — verify it by running with the host binary removed or masked off PATH.
script/lint runs the digest-pinned golangci/golangci-lint image, mounting the source read-only where practical and a writable cache directory so repeat runs stay fast. Report warm and cold timings; make check must stay within the 20-second budget on a warm cache.
script/bootstrap no longer go installs golangci-lint. Keep whatever it still legitimately does for host tooling, and update its comments so they do not describe an install that no longer happens.
The image is pinned by digest with the version/date comment REPO_POLICIES requires, and the pin remains single-source — it is currently in script/bootstrap and consumed by script/verify-linter-pin. Decide where it lives now and make sure the Dockerfile lint stage, script/lint and the guard cannot drift apart. Do not create a third copy.
script/verify-linter-pin is kept and still guards the binary the Dockerfile build stage receives, or is deliberately retired with the reasoning recorded. Do not silently drop it — it was added in #42 precisely to stop the two stages diverging.
The Dockerfile lint stage keeps working and still gates the build stage. Prove it the way previous reviews did: plant a lint finding and confirm the build fails before make check starts.
make check green, make docker green with the gates demonstrably executing (#32, #39), and the non-root quirk intact — --user 0:0 must still fail TestScanHardlinkRunFailsTogether with the test cache disabled.
README §Entrypoints updated to describe how linting now runs, including the Docker requirement.

Worth flagging as an open question rather than assuming: script/lint invoking Docker means make check requires Docker, and make docker runs make check inside a container. Make sure that does not become a nested-Docker requirement in the image build — the containerised lint path likely needs to be skipped or short-circuited when already running inside the lint stage. This is the trap in this issue.

Owner instruction, given directly on PR #44 (https://git.eeqj.de/sneak/sfdupes/pulls/44#issuecomment-50463): > linting should always happen in a container, never installed on the dev environment. assume we have docker. Current state contradicts that. `script/lint` runs whatever `golangci-lint` is on `PATH`, and `script/bootstrap` `go install`s it onto the host. This supersedes the approach taken in #24 rather than invalidating it. #24 existed because the host linter could silently be a different version than CI — this removes the host linter entirely, which dissolves that whole class of problem instead of policing it. Several open issues shrink or disappear as a consequence, and should be re-read after this lands: - **#36** (linter cache attributing findings to deleted worktree paths) largely evaporates if the linter runs in a container with a controlled cache mount. - The host-side half of **#24** — version parsing, install verification, `PATH` shadowing — becomes dead weight once nothing installs the linter on the host. `script/verify-linter-pin` stays useful for the in-image copy. - **#19** (Markdown formatting dropped from `script/fmt`) intersects: if prettier is also containerised, that removes the awkward question of how a Go repo obtains a Node tool, which was the hard part of that issue. ## Definition of done 1. `make lint` works on a machine with Docker and no `golangci-lint` installed. That is the acceptance test — verify it by running with the host binary removed or masked off `PATH`. 2. `script/lint` runs the digest-pinned `golangci/golangci-lint` image, mounting the source read-only where practical and a writable cache directory so repeat runs stay fast. Report warm and cold timings; `make check` must stay within the 20-second budget on a warm cache. 3. `script/bootstrap` no longer `go install`s golangci-lint. Keep whatever it still legitimately does for host tooling, and update its comments so they do not describe an install that no longer happens. 4. The image is pinned by digest with the version/date comment REPO_POLICIES requires, and the pin remains single-source — it is currently in `script/bootstrap` and consumed by `script/verify-linter-pin`. Decide where it lives now and make sure the Dockerfile lint stage, `script/lint` and the guard cannot drift apart. Do not create a third copy. 5. `script/verify-linter-pin` is kept and still guards the binary the Dockerfile build stage receives, or is deliberately retired with the reasoning recorded. Do not silently drop it — it was added in #42 precisely to stop the two stages diverging. 6. The Dockerfile lint stage keeps working and still gates the build stage. Prove it the way previous reviews did: plant a lint finding and confirm the build fails before `make check` starts. 7. `make check` green, `make docker` green with the gates demonstrably executing (#32, #39), and the non-root quirk intact — `--user 0:0` must still fail `TestScanHardlinkRunFailsTogether` with the test cache disabled. 8. README §Entrypoints updated to describe how linting now runs, including the Docker requirement. Worth flagging as an open question rather than assuming: `script/lint` invoking Docker means `make check` requires Docker, and `make docker` runs `make check` inside a container. Make sure that does not become a nested-Docker requirement in the image build — the containerised lint path likely needs to be skipped or short-circuited when already running inside the lint stage. This is the trap in this issue.

clawbot added this to the 1.0.0 milestone 2026-08-09 17:44:22 +02:00

clawbot referenced this issue

2026-08-10 14:28:58 +02:00

Run all linting in Docker via Dockerfile.lint + script/lint #46

clawbot commented

2026-08-10 14:29:05 +02:00

Superseded by #46, which
carries the owner's ruling on the exact shape (root Dockerfile.lint,
script/lint reduced to building it). The acceptance criteria here —
the nested-Docker trap, the fate of script/verify-linter-pin, the
single-source pin, the non-root quirk, the README entry — are folded
into the implementation requirements there. Closing to keep one issue
for one unit of work.

Superseded by https://git.eeqj.de/sneak/sfdupes/issues/46, which carries the owner's ruling on the exact shape (root `Dockerfile.lint`, `script/lint` reduced to building it). The acceptance criteria here — the nested-Docker trap, the fate of `script/verify-linter-pin`, the single-source pin, the non-root quirk, the README entry — are folded into the implementation requirements there. Closing to keep one issue for one unit of work.

clawbot closed this issue

2026-08-10 14:29:08 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/sfdupes#45