ci: lint the issues a change introduces, not nothing at all - #48
Open
blairham wants to merge 4 commits into
Open
ci: lint the issues a change introduces, not nothing at all#48blairham wants to merge 4 commits into
blairham wants to merge 4 commits into
Conversation
The `golangci-lint` pre-commit hook reports nothing on a CI runner. Its upstream entry is `--new-from-rev HEAD`, which compares the *working tree* against HEAD: exactly right locally, where it names what you are about to commit and `--fix` corrects it, and empty on a runner, where the checkout is clean. Measured on a tree carrying a real errcheck violation, the hook passed while `golangci-lint run` reported two issues on the same tree. `pass_filenames: false` is why diff-scoping the pre-commit *run* cannot help: pre-commit passes no filenames in any mode, so the entry's own base rev is the only thing that decides. The fix has to change the base rev. So the same upstream hook is declared a second time under an alias, with `--new-from-merge-base=origin/main` appended — args are appended to the entry and the later flag wins — and `stages: [manual]` to keep it out of every local commit, where the hook above already does the right thing. No `repo: local` hook, and no second lint authority: same config, same pin, same hook, asked a question it can answer on a clean checkout. The `rev-parse --verify` step is the point, not ceremony. A base ref that does not resolve makes golangci-lint print `0 issues` and exit 0 rather than erroring, so a shallow clone would turn this into a green check that linted nothing — the exact failure being fixed. `fetch-depth: 0` supplies the base branch history the merge base needs, and the assertion fails loudly if it ever goes missing. The comments claiming CI already lints the change against its base described something that was never true; they are corrected here.
Two rules, applied everywhere: CI runs against the diff and never against every file, and CI reports rather than repairs. Both were broken here. `--all-files` is gone from CI. Every run is now scoped with `--from-ref`/`--to-ref` to the range the push or pull request actually introduced, resolved once in a step that falls back to the merge base with main when `before` arrives as all zeros (branch create, force-push) and then verifies every ref resolves. That verification is the point: an unresolvable ref makes both pre-commit and golangci-lint report nothing and exit 0, so without it a lost `fetch-depth: 0` becomes a green check over an unlinted change. The old default was `--all-files`, which replayed the whole repository's backlog at every contributor and reported it as if this change had caused it. `--fix=false` is appended to the CI hook's args. Upstream's entry carries `--fix`, and a linter that repairs its own findings has nothing left to report: measured on a committed misspelling, golangci-lint rewrote the runner's working copy, printed `0 issues` and exited 0. Green check, no output, and the committed code still misspelled — the same silent pass this work exists to end, one layer down. Fixing belongs to the local hook, which runs against the working tree where the repair is what you want; CI only reports. Verified both directions on the same content: the local hook corrected it in place and passed, the CI hook named both issues, exited 1 and left the file untouched. `--show-diff-on-failure` is added so the hooks that can only work by fixing (end-of-file-fixer and friends) surface as a readable diff instead of a bare red check. In pre-commit-hooks the Python parity smoke test is diff-scoped too. Parity is that job's whole purpose, so it has to ask both runners the same question; an `--all-files` run there compared a different one.
The action's own run line is already
`pre-commit run --show-diff-on-failure --color=always ${{ inputs.extra_args }}`,
so passing the flag through `extra_args` just repeated it on the command line.
Harmless but misleading: it reads as though the diff output depended on this
change. It does not — only the Python parity job in pre-commit-hooks, which
invokes `pre-commit` directly rather than through the action, still needs to
pass it itself.
A fixing hook normally cannot hide in CI: pre-commit fails it with "files were modified by this hook", which is why end-of-file-fixer and friends stay honest without any extra flag. That check works from the filenames pre-commit handed the hook, and golangci-lint is `pass_filenames: false`, so there are none to compare against and the net does not catch it. Measured both ways on the same runner: end-of-file-fixer failed on a file it fixed, golangci-lint reported Passed on a file it fixed. Without that contrast the `--fix=false` line reads like belt-and-braces and is a natural thing for someone to delete later.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
The
golangci-lintpre-commit hook reports nothing on a CI runner. Measuredon a tree carrying a real errcheck violation:
go tool golangci-lint runpre-commit run golangci-lint --all-filespre-commit run --from-ref BASE --to-ref HEADUpstream's entry is
golangci-lint run --new-from-rev HEAD --fixwithpass_filenames: false.--new-from-rev HEADcompares the working treeagainst HEAD — exactly right locally, and empty on a runner where the checkout
is clean. And because
pass_filenames: false, diff-scoping the pre-commit runcannot help: pre-commit passes no filenames in any mode, so the entry's own base
rev is the only thing that decides.
The trap: a base ref that does not resolve makes golangci-lint print
0 issuesand exit 0. It does not error. A shallow clone or an empty baseref therefore yields a green check that linted nothing.
The fix
The same upstream hook, declared a second time under an alias with the base it
can actually use. No
repo: localhook, and no second lint authority.args:are appended to the entry, so the command becomesgolangci-lint run --new-from-rev HEAD --fix --new-from-merge-base=origin/mainand
--new-from-merge-basewins (measured).stages: [manual]keeps it outof every local commit, where the hook above already reports what you are about
to write and
--fixcorrects it.In CI:
fetch-depth: 0supplies the base history the merge base needs,SKIP: golangci-lintdrops the run that can only report nothing, and agit rev-parse --verify origin/mainstep fails loudly rather than letting amissing base become a silent pass.
Verified
pre-commit run --all-filesgolangci-lint-fmt+golangci-lint0 issuesgit rev-parse --verify origin/mainKnown limit
The base is a static
origin/mainrather than${{ github.base_ref }}, whichcan arrive empty and silently lint nothing. The cost: a merge to main lints
nothing, because merge-base == HEAD. Pull requests are covered; main is not. If
that matters, a follow-up can add upstream's
golangci-lint-fullas a secondmanual hook run only on push.