Skip to content

golangci: run the linters this file claims to configure - #665

Merged
Philip Lombardi (plombardi89) merged 1 commit into
mainfrom
fix/golangci-config-enforces-what-it-says
Aug 26, 2026
Merged

golangci: run the linters this file claims to configure#665
Philip Lombardi (plombardi89) merged 1 commit into
mainfrom
fix/golangci-config-enforces-what-it-says

Conversation

@plombardi89

Copy link
Copy Markdown
Collaborator

Fixes #657.

.golangci.yaml never set linters.default, so it took golangci-lint v2's implicit standard set. Seven linters ran; the rest of the file was inert:

Location Configured Status
:35-63 godot, godox, nolintlint, varnamelen settings linters never ran
:15-24 disable: gocognit, gomoddirectives already off under standard
:79-84 _test.go exclusion 3 of 4 linters named were off
:99-104 formatters.settings.gci gci absent from formatters.enable
:6 run.build-tags: int no //go:build int in the tree

What changed

default: none, every linter named. Listing the five standard linters costs five lines and buys two things: a golangci-lint bump can't change what runs without a diff here, and "every settings block belongs to an enabled linter" becomes checkable by reading the file.

Eighteen linters enabled, each verified at zero findings individually against the pinned v2.13.1: asasalint, asciicheck, bidichk, durationcheck, exptostd, fatcontext, gocheckcompilerdirectives, iface, interfacebloat, loggercheck, misspell, nakedret, nilnesserr, reassign, spancheck, tagalign, testableexamples, whitespace.

decorder also reports zero and is deliberately excluded: every one of its checks defaults to disable-*: true, so enabling it would re-create exactly the problem this PR fixes.

gci enabled. Not cosmetic. goimports sorts within the groups a file already has but never moves an import between them, so the prefix(github.com/Azure/unbounded) section was advisory and first-party imports had settled into the third-party group. Positive control on v2.13.1:

# without gci                          # with gci
import (                               import (
    "context"                              "context"

    inv "github.com/Azure/unbounded/…"     "google.golang.org/grpc"

    "google.golang.org/grpc"               inv "github.com/Azure/unbounded/…"
)                                      )

That accounts for all 21 Go files in this diff. They are imports only, no logic.

./deploy/... added to GO_PACKAGE_PATTERNS. The embed.go files and the render tests guarding the shipped manifests were linted by nothing. They already pass, and the packages load on a fresh clone by design (deploy/machina/embed.go:11-17).

Not in scope

The linters with real findings land separately, per #657. Starting batch is nilerr (14) and predeclared (20), which the issue calls out. nilerr is not all noise: internal/net/controller/site_controller.go:877 treats every lister error as "node was deleted", internal/operator/migrate.go:1666 returns false, nil on err != nil, and internal/net/netlink/link_manager.go:295 treats any LinkByName failure as "doesn't exist". The tree also already carries 5 //nolint:nilerr and 3 //nolint:nilnil directives written against linters that had never run.

Found while doing this, filed separately

make fmt has a latent race. GO_PACKAGE_DIRS contains nested package dirs and gofumpt recurses, so nested files get visited once per ancestor, concurrently. It's masked only because gofumpt currently has nothing to rewrite; an experiment that made it rewrite files hit it immediately:

error: size of hack/cmd/relctl/relctl/version/resolve.go changed during reading
make: *** [Makefile:508: fmt] Error 2

Pre-existing and unrelated to #657, so not fixed here.

Verification

Against the pinned v2.13.1 (Makefile:435), not a locally-installed version — the schema for formatters.settings.gofumpt changed between v2.11 and v2.13, and reading the wrong one sends you the wrong way.

  • golangci-lint config verify passes; it did not before.
  • make lint clean, make fmt idempotent, go build ./... clean.
  • Tests pass for all 21 affected packages.

.golangci.yaml never set `linters.default`, so it took golangci-lint v2's
implicit `standard`: errcheck, govet, ineffassign, staticcheck, unused,
plus the two enabled by name. Everything else in the file was inert. The
settings blocks for godot, godox, nolintlint and varnamelen configured
linters that never ran, the `disable` entries for gocognit and
gomoddirectives turned off linters that were already off, and three of
the four linters named in the `_test.go` exclusion were not running to be
excluded. A green `make lint` said less than the file implied, which is
roughly how it gets cited in review.

Set `default: none` and name every linter, including the five from
`standard`. Listing them costs five lines and buys two things: a
golangci-lint bump can no longer change what runs without a diff here,
and "every settings block belongs to an enabled linter" becomes a
property you can check by reading the file rather than by running the
tool.

Enable the eighteen linters that report nothing on the tree as it stands,
verified one at a time against the pinned v2.13.1. They are on to hold
the line, not to work through a backlog. Note that decorder also reports
nothing and is deliberately not among them: every one of its checks
defaults to disable-*: true, so enabling it would have re-created exactly
the problem this commit fixes. The linters with real findings, nilerr and
predeclared first, are tracked in #657 and land separately.

The dead settings blocks are deleted rather than commented out. The
rationale that was worth keeping, why gocognit and gomoddirectives will
not be enabled, is now a comment that cannot be mistaken for enforcement.

Enable gci, whose settings block was inert because gci was absent from
`formatters.enable`. This one was not cosmetic: goimports sorts within
the groups a file already has but will not move an import between them,
so the `prefix(github.com/Azure/unbounded)` section was advisory and
first-party imports had settled into the third-party group in 21 files.
Those are the only Go changes here, and they are imports only.

Add ./deploy/... to GO_PACKAGE_PATTERNS. The embed.go files and the
render tests that guard the shipped manifests were linted by nothing.
They already pass; the packages load on a fresh clone by design, per the
comment in deploy/machina/embed.go.

Drop the `int` build tag, which no file uses, and the `-E wsl_v5` from
`make fmt`, which re-enables a linter the config already enables while
implying the other auto-fixable ones are not applied.

Note for anyone measuring a follow-up batch: per-linter counts have to be
taken one linter at a time. `--default=all --show-stats` undercounts,
because `uniq-by-line` drops all but one issue per line. predeclared
reads as 0 findings that way and has 20 when run alone.
@plombardi89
Philip Lombardi (plombardi89) requested a review from a team August 25, 2026 22:08
@plombardi89
Philip Lombardi (plombardi89) added this pull request to the merge queue Aug 26, 2026
Merged via the queue into main with commit e284b3b Aug 26, 2026
32 checks passed
@plombardi89
Philip Lombardi (plombardi89) deleted the fix/golangci-config-enforces-what-it-says branch August 26, 2026 14:45
Philip Lombardi (plombardi89) added a commit that referenced this pull request Aug 26, 2026
Conflict was confined to .golangci.yaml, against #665, which set
`linters.default: none` and named every linter explicitly.

#665 already enables misspell, but with no settings block it runs at the
default locale, where it only corrects unambiguous typos and passes
British spellings straight through. That is why it reported nothing on a
tree that still had ~370 of them. `locale: US` is the part that makes it
enforce anything, so this keeps that settings block and drops the
nolintlint and varnamelen blocks #665 deleted as settings for linters
that never run.

The auto-merge also produced a duplicate `misspell` in `linters.enable`,
one entry from each side. Resolved to a single entry under "Project
policy" rather than in the zero-config hygiene group: it now carries a
settings block, which is what goheader and wsl_v5 have in common.

Took #665's errcheck-only `_test.go` exclusion, and kept the
hack/cmd/notice exclusion. The latter satisfies #665's new rule that
exclusions only ever name enabled linters, and it stays scoped by `text`
so it exempts the LICENCE filenames rather than the whole package: a
`behaviour` in that package is still caught.

Also fixes `recognise` in the file header comment #665 added. misspell
does not read YAML, which is the case for stating the rule in AGENTS.md
rather than leaving it to the tool.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

golangci-lint: most of .golangci.yaml never runs

2 participants