sneak/dnswatcher
1
0
Fork 0
Code Issues 29 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

Stale/incorrect comment text in script/bootstrap and script/cibuild, and record the config-verify tradeoff #137

New Issue
Closed
opened 2026-08-10 14:50:03 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-10 14:50:03 +02:00
Collaborator
Copy Link
Copy Source

Follow-up to the review of #136. All comment-only, no behavioural effect — which is why they did not fail that review.

Definition of done

  1. script/bootstrap header is false. It justifies keeping the goimports install by saying "script/fmt and script/fmt-check run it on the host". script/fmt-check runs only gofmt -l . — it does not use goimports. The justification was introduced by the Docker-lint commit and repeated in its commit message and PR body. Correct the header to state the real reason (script/fmt alone).

    Note there is a live question underneath this: #119 records that fmt-check does not verify goimports at all, which is arguably the actual defect. Fix only the false comment here; leave the behaviour to that issue.

  2. script/cibuild header is stale. It still says "The Dockerfile runs make check". It no longer does — after #136 the builder stage runs make fmt-check, make test, and make build, with linting in a separate stage. Every other doc was updated; this one was missed.

  3. The docker-missing warning in script/bootstrap is wrapped mid-clause with the bootstrap: prefix repeated inside the broken sentence. Cosmetic; make it read as one sentence.

  4. Record the config verify tradeoff in the comment at the top of Dockerfile.lint. That comment currently explains why golangci-lint config verify is omitted (it fetches its JSON schema over an unpinned live HTTPS call, which would make linting network-dependent and defeat hash-pinning). The reasoning is correct and the decision stands, but the omission is not free and the comment reads as though it is: unknown top-level keys in .golangci.yml are silently ignored. Demonstrated during review — a bogus top-level key was appended and make lint returned exit 0, 0 issues.

    This repo has already been bitten by exactly this failure mode: the comment at .golangci.yml lines 3-5 exists because a v1-schema linters-settings block was silently not applied under the v2 schema. State the residual risk plainly so a future reader knows a config typo will pass silently rather than assuming the omission is cost-free.

make check green. Commit title ends with (closes #N) for this issue.

Follow-up to the review of https://git.eeqj.de/sneak/dnswatcher/pulls/136. All comment-only, no behavioural effect — which is why they did not fail that review. ## Definition of done 1. **`script/bootstrap` header is false.** It justifies keeping the `goimports` install by saying "`script/fmt` and `script/fmt-check` run it on the host". `script/fmt-check` runs only `gofmt -l .` — it does not use goimports. The justification was introduced by the Docker-lint commit and repeated in its commit message and PR body. Correct the header to state the real reason (`script/fmt` alone). Note there is a live question underneath this: https://git.eeqj.de/sneak/dnswatcher/issues/119 records that `fmt-check` does not verify goimports at all, which is arguably the actual defect. Fix only the false comment here; leave the behaviour to that issue. 2. **`script/cibuild` header is stale.** It still says "The Dockerfile runs make check". It no longer does — after https://git.eeqj.de/sneak/dnswatcher/pulls/136 the builder stage runs `make fmt-check`, `make test`, and `make build`, with linting in a separate stage. Every other doc was updated; this one was missed. 3. **The `docker`-missing warning in `script/bootstrap`** is wrapped mid-clause with the `bootstrap:` prefix repeated inside the broken sentence. Cosmetic; make it read as one sentence. 4. **Record the `config verify` tradeoff** in the comment at the top of `Dockerfile.lint`. That comment currently explains why `golangci-lint config verify` is omitted (it fetches its JSON schema over an unpinned live HTTPS call, which would make linting network-dependent and defeat hash-pinning). The reasoning is correct and the decision stands, but the omission is not free and the comment reads as though it is: **unknown top-level keys in `.golangci.yml` are silently ignored.** Demonstrated during review — a bogus top-level key was appended and `make lint` returned exit 0, `0 issues.` This repo has already been bitten by exactly this failure mode: the comment at `.golangci.yml` lines 3-5 exists because a v1-schema `linters-settings` block was silently not applied under the v2 schema. State the residual risk plainly so a future reader knows a config typo will pass silently rather than assuming the omission is cost-free. `make check` green. Commit title ends with ` (closes #N)` for this issue.
clawbot commented 2026-08-10 16:10:29 +02:00
Author
Collaborator
Copy Link
Copy Source

Landed on next as b8662b8, carried by #136.

The four texts as they now read:

  1. script/bootstrap header: "goimports is installed via go install at a pinned commit (never "latest") because script/fmt runs it on the host; script/fmt-check does not (it runs gofmt only)." Verified by reading script/fmt-check: its only formatting step is gofmt -l .. Behaviour untouched — that fmt-check never verifies goimports stays with #119.
  2. script/cibuild header: "run the CI build. The Dockerfile's lint stage runs make fmt-check and golangci-lint; its builder stage runs make test and make build. A successful build implies all of those passed." Taken from the current Dockerfile, not from the issue text.
  3. docker-missing warning, now one sentence emitted by a single echo: bootstrap: WARNING: docker not found; install it to run make lint and make docker.
  4. Dockerfile.lint header, appended to the existing config verify paragraph: "The cost of that: unknown top-level keys in .golangci.yml are silently ignored, so a mistyped or wrong-schema key lints clean while applying nothing." config verify was not added.

Verification: make check green, exit 0. Tests ran rather than replayed (zero (cached) lines, internal/resolver 2.820s); the Docker lint stage executed rather than cached (#10 [lint 2/2] RUN golangci-lint run ... 0 issues. after #8 ... CACHED for go mod download). The full diff is comment lines, the warning string, and TODO.md — no statement, flag, or command changed.

Landed on `next` as `b8662b8`, carried by https://git.eeqj.de/sneak/dnswatcher/pulls/136. The four texts as they now read: 1. `script/bootstrap` header: "goimports is installed via `go install` at a pinned commit (never "latest") because script/fmt runs it on the host; script/fmt-check does not (it runs gofmt only)." Verified by reading `script/fmt-check`: its only formatting step is `gofmt -l .`. Behaviour untouched — that `fmt-check` never verifies goimports stays with https://git.eeqj.de/sneak/dnswatcher/issues/119. 2. `script/cibuild` header: "run the CI build. The Dockerfile's lint stage runs make fmt-check and golangci-lint; its builder stage runs make test and make build. A successful build implies all of those passed." Taken from the current `Dockerfile`, not from the issue text. 3. `docker`-missing warning, now one sentence emitted by a single `echo`: `bootstrap: WARNING: docker not found; install it to run make lint and make docker.` 4. `Dockerfile.lint` header, appended to the existing `config verify` paragraph: "The cost of that: unknown top-level keys in .golangci.yml are silently ignored, so a mistyped or wrong-schema key lints clean while applying nothing." `config verify` was not added. Verification: `make check` green, exit 0. Tests ran rather than replayed (zero `(cached)` lines, `internal/resolver 2.820s`); the Docker lint stage executed rather than cached (`#10 [lint 2/2] RUN golangci-lint run ... 0 issues.` after `#8 ... CACHED` for `go mod download`). The full diff is comment lines, the warning string, and `TODO.md` — no statement, flag, or command changed.
sneak closed this issue 2026-09-09 14:57:56 +02:00
sneak referenced this issue from a commit 2026-09-09 14:57:57 +02:00
next (#136)
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#137