Stale/incorrect comment text in script/bootstrap and script/cibuild, and record the config-verify tradeoff #137
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
script/bootstrapheader is false. It justifies keeping thegoimportsinstall by saying "script/fmtandscript/fmt-checkrun it on the host".script/fmt-checkruns onlygofmt -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/fmtalone).Note there is a live question underneath this: #119 records that
fmt-checkdoes not verify goimports at all, which is arguably the actual defect. Fix only the false comment here; leave the behaviour to that issue.script/cibuildheader is stale. It still says "The Dockerfile runs make check". It no longer does — after #136 the builder stage runsmake fmt-check,make test, andmake build, with linting in a separate stage. Every other doc was updated; this one was missed.The
docker-missing warning inscript/bootstrapis wrapped mid-clause with thebootstrap:prefix repeated inside the broken sentence. Cosmetic; make it read as one sentence.Record the
config verifytradeoff in the comment at the top ofDockerfile.lint. That comment currently explains whygolangci-lint config verifyis 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.ymlare silently ignored. Demonstrated during review — a bogus top-level key was appended andmake lintreturned exit 0,0 issues.This repo has already been bitten by exactly this failure mode: the comment at
.golangci.ymllines 3-5 exists because a v1-schemalinters-settingsblock 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 checkgreen. Commit title ends with(closes #N)for this issue.Landed on
nextasb8662b8, carried by #136.The four texts as they now read:
script/bootstrapheader: "goimports is installed viago installat a pinned commit (never "latest") because script/fmt runs it on the host; script/fmt-check does not (it runs gofmt only)." Verified by readingscript/fmt-check: its only formatting step isgofmt -l .. Behaviour untouched — thatfmt-checknever verifies goimports stays with #119.script/cibuildheader: "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 currentDockerfile, not from the issue text.docker-missing warning, now one sentence emitted by a singleecho:bootstrap: WARNING: docker not found; install it to run make lint and make docker.Dockerfile.lintheader, appended to the existingconfig verifyparagraph: "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 verifywas not added.Verification:
make checkgreen, 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 ... CACHEDforgo mod download). The full diff is comment lines, the warning string, andTODO.md— no statement, flag, or command changed.