Skip to content

ci(lint): adopt strict golangci-lint config (default:all) and clean up resulting backlog #153

@lexfrei

Description

@lexfrei

Summary

golangci-lint is currently run with an implicit minimal config — only the default linter set fires, and the project carries a small backlog of pre-existing findings nobody owns. Adopt a serious default: all config (with sensible relaxations for tests and generated code), then clean up everything the broader rule set surfaces. The three unusedparams warnings that prompted this issue are a representative slice — the same pass will surface a longer list across pkg/engine, pkg/commands, pkg/age, etc.

Currently observable findings

Even on the current weak config, three unusedparams warnings show up:

File Line Unused parameter
pkg/commands/apply.go 96 args
pkg/commands/rotate_ca_handler.go 317 targetNode
pkg/commands/template.go 219 args

Switching to default: all will reveal more. The cleanup is part of this issue, not a separate one.

Proposed .golangci.yml

version: "2"

linters:
  default: all
  disable:
    - depguard
    - exhaustruct
    - gochecknoinits
    - wsl
    - lll
    - errchkjson
    - ireturn
    - gocheckcompilerdirectives
  settings:
    dupl:
      threshold: 100
    goconst:
      min-len: 2
      min-occurrences: 2
    gocritic:
      disabled-checks:
        - dupImport
        - unnamedResult
      enabled-tags:
        - diagnostic
        - experimental
        - opinionated
        - performance
        - style
    funlen:
      lines: 60
      statements: 60
    gocyclo:
      min-complexity: 15
    cyclop:
      max-complexity: 15
    mnd:
      ignored-numbers:
        - "10"
        - "100"
        - "1000"
        - "2"
        - "60"
        - "60.0"
        - "64"
        - "500"
    nolintlint:
      require-explanation: true
      require-specific: true
      allow-unused: false
    varnamelen:
      max-distance: 5
      min-name-length: 3
      check-receiver: false
      check-return: false
      ignore-type-assert-ok: false
      ignore-map-index-ok: false
      ignore-chan-recv-ok: false
      ignore-decls:
        - wg sync.WaitGroup
        - wg *sync.WaitGroup
        - mu sync.Mutex
        - ok bool
      ignore-names:
        - i
        - w
        - r
        - b
        - c
        - m
        - n
        - tt
        - rw
  exclusions:
    generated: lax
    presets:
      - comments
      - common-false-positives
      - legacy
      - std-error-handling
    paths:
      - third_party$
      - builtin$
      - generated\.go$
      - pkg/generated/
    rules:
      - linters:
          - funlen
          - dupl
          - gocognit
          - gocyclo
          - cyclop
          - errcheck
          - testableexamples
          - testpackage
          - forcetypeassert
          - gocritic
          - nlreturn
          - wsl_v5
          - varnamelen
          - unparam
          - modernize
          - gosec
          - testifylint
          - perfsprint
          - paralleltest
          - maintidx
        path: _test\.go

formatters:
  enable:
    - gofmt
    - gofumpt
    - goimports
  exclusions:
    generated: lax
    paths:
      - third_party$
      - builtin$
      - generated\.go$
      - pkg/generated/

Notes:

  • default: all opts in to every linter that ships with golangci-lint v2; the disable list removes the ones that consistently fight Go idioms (depguard requires hand-curated allowlists, exhaustruct rejects partial struct literals which Go callers use everywhere, wsl enforces blank-line aesthetics, lll redundant with most editors).
  • Per-test relaxations under path: _test\.go keep the test-writing experience tractable: long table-driven tests, intentional duplication across cases, type assertions in fixtures.
  • pkg/generated/ is excluded entirely (generated-from-Helm-charts assets).
  • nolintlint requires every //nolint: directive to name specific linters and explain itself — prevents blanket suppressions.

Approach

Suggested order of work:

  1. Land .golangci.yml as proposed above (or with project-specific tweaks). Run golangci-lint run ./... once to enumerate the resulting backlog.
  2. Triage the backlog into three buckets:
    • Trivially mechanical (unused params, missing docstrings, error returns ignored) — fix in one batch.
    • Worth owning (high-cyclomatic functions, magic numbers, naming) — split into targeted PRs by area.
    • Genuinely expensive to fix (large refactors, public API impact) — file follow-up issues with kind/cleanup and the relevant area/*, then add path-specific exclusions in .golangci.yml so CI stays green in the meantime. Each exclusion must reference the follow-up issue in a comment.
  3. Wire golangci-lint run into CI so future PRs cannot reintroduce findings.

The three unusedparams warnings listed at the top of this issue are eligible for the first bucket — fix in the same PR that lands the config.

Out of scope

  • New tests (covered by separate ongoing test-coverage work).
  • Code refactors beyond what individual lint rules ask for.
  • Changing public API signatures (a breaking change for external talm consumers — defer).

Notes

The currently disabled set in this proposal is a starting point. Project maintainers can add or remove based on Go style preferences (e.g. wsl_v5 for blank-line discipline if preferred, mnd ignored-numbers list for project-specific magic constants).

Metadata

Metadata

Assignees

Labels

kind/cleanupCategorizes issue or PR as related to cleanup of code, process, or technical debtpriority/backlogGeneral backlog priority. Lower than priority/important-longtermtriage/acceptedIndicates an issue is ready to be actively worked on

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions