feat(linters): Added golangci-lint config & CI job#304
feat(linters): Added golangci-lint config & CI job#304Leeaandrob merged 12 commits intosipeed:mainfrom
Conversation
|
@Zepan This PR adds Recommendation: Merge. This is infrastructure that pays dividends on every future PR. The roadmap explicitly calls for better CI/CD (volunteer role #5: "AI-Powered CI/CD Optimization"). |
Leeaandrob
left a comment
There was a problem hiding this comment.
Review: PR #304 — feat(linters): Added golangci-lint config & CI job
@mymmrac Solid infrastructure PR. The incremental approach (start with default: all, disable 2600+ existing issues, re-enable step by step) is the right strategy for a fast-growing project with 68+ open PRs.
Local Verification
| Check | Result |
|---|---|
go vet ./... |
PASS |
go build ./... |
PASS |
go test -race ./... |
PASS (all packages) |
| CI checks | 4/4 PASS (Linter, Formatting, Vet, Tests) |
Findings
[MEDIUM] — version: latest + default: all = flaky CI risk (.github/workflows/pr.yml:25)
The combination of version: latest for the golangci-lint binary and default: all in the config creates a fragile pairing. When golangci-lint releases a new version that introduces a new linter, it will be auto-enabled and could fail on existing code — breaking ALL PRs until someone adds it to the disable list.
Suggestion: Pin to a specific version (e.g., version: v2.1) or at least a major version range. This way CI only breaks when you intentionally upgrade.
# Instead of:
version: latest
# Use:
version: v2.1[MEDIUM] — Critical linters all disabled
The following high-value linters are currently disabled: govet, staticcheck, errcheck, unused, ineffassign, gosec. This is clearly intentional (2600+ existing issues), and the TODOs document the plan to re-enable them. However, I'd recommend prioritizing these 6 first:
govet— catches real bugs (already running as separate CI job anyway)staticcheck— the gold standard for Go analysiserrcheck— unchecked errors are a top Go bug categoryunused— dead code removal keeps codebase cleanineffassign— subtle bugs from shadowed assignmentsgosec— security linter, critical for a project handling user credentials/API keys
Suggestion: Consider enabling govet now (in this PR) since it already passes in the separate vet CI job. That way you immediately get value from golangci-lint beyond what the existing CI already checks.
[LOW] — Redundant vet and fmt-check jobs
Once golangci-lint is properly configured, the separate vet and fmt-check jobs become redundant (golangci-lint subsumes both). The TODOs already document this — just confirming the plan is sound.
[LOW] — goimports formatter enabled but other formatters commented out
The gci, gofmt, gofumpt, golines formatters are commented out with a TODO. Consider enabling at least gofmt alongside goimports since the Makefile's fmt target already runs go fmt ./... — this would ensure consistency between local make fmt and CI checks.
Positives
- [POSITIVE] Clean separation between "project preference" disables and "temporarily failing" disables — makes the roadmap clear
- [POSITIVE] Action version bumps (
checkout@v4→v6,setup-go@v5→v6) across all 4 workflows — good housekeeping - [POSITIVE] Test file exclusions for complexity linters (
funlen,maintidx,gocognit,gocycloon_test.go) — pragmatic - [POSITIVE] Revive rule configuration is well-tuned (disabled
add-constant,cognitive-complexity, sensibleargument-limit: 7) - [POSITIVE]
issues.max-issues-per-linter: 0andmax-same-issues: 0— full transparency, no hidden failures - [POSITIVE] Using
go-version-file: go.modinstead of hardcoded version — single source of truth
Verdict: APPROVE
This is foundational CI infrastructure. The approach is methodical, the config is well-organized, and the TODOs create a clear improvement roadmap. The version: latest risk is the only thing I'd push to address before merge — everything else can follow in subsequent PRs as planned.
Good work on this — it will pay dividends on every future PR.
| - name: Run go generate | ||
| run: go generate ./... | ||
|
|
||
| - name: Golangci Lint |
There was a problem hiding this comment.
[MEDIUM] version: latest combined with default: all in .golangci.yaml means any new linter introduced in a golangci-lint release will be auto-enabled and could break all PRs.
Suggestion: Pin to a specific version:
version: v2.1This way CI upgrades are intentional, not accidental.
| - godox | ||
| - goprintffuncname | ||
| - gosec | ||
| - govet |
There was a problem hiding this comment.
[MEDIUM] govet is disabled here but already runs successfully as a separate CI job (vet in pr.yml). Consider enabling it now to get immediate value from golangci-lint — it's a zero-risk enable since the separate vet job already proves it passes.
Once enabled here, the separate vet job can be removed (as noted in the TODO).
| @@ -0,0 +1,184 @@ | |||
| version: "2" | |||
There was a problem hiding this comment.
[POSITIVE] Using golangci-lint v2 config format with default: all is the right call. The two-block disable strategy (permanent project preferences vs temporary failures) makes the improvement roadmap very clear.
Nice config overall — the revive rules, test exclusions, and formatter setup are all well-tuned.
|
@mymmrac take look. After that we can accept without problem. |
|
Thanks for the review, from the comment all apart from fixed version is by design, if I will enable all linters and formatters right away we will have a PR that touches almost all files in the project I will make linter version fixed, if you see a need for change in anything else let's discuss |
…ackages Fixes identified by running golangci-lint v2.10.1 (PR sipeed#304 config) with govet, staticcheck, errcheck, revive, gosec enabled: - Replace interface{} with any (revive: use-any) - Replace WriteString(fmt.Sprintf(...)) with fmt.Fprintf (staticcheck: QF1012) - Add doc comments on all exported methods (revive: exported) - Safe type assertions with ok-check pattern (errcheck/revive) - Use strings.EqualFold instead of double ToLower (staticcheck: SA6005) - Add default cases to switch statements (revive) - Suppress gosec G117 false positive on SessionKey field - Test improvements: range int, unused params Zero issues remaining in pkg/multiagent, pkg/agent, pkg/routing. All 47 tests pass. Relates to sipeed#304, sipeed#294
feat(linters): Added golangci-lint config & CI job
This PR adds golangci-lint config, it's not final and needs tweaking, but it should be a good starting point,
For now most linters are disabled (because we have 2600+ issues currently) and we should enable them as we fix them.