chore(lint): bump golangci lint config to v2, address lint findings #365
Conversation
mattsre
force-pushed
the
golang-lintfix
branch
from
3bcfe35 to
608024c
Compare
| if topicMeta.Partitions > 0 { | ||
| nonPartitioned = false | ||
| } | ||
| nonPartitioned := topicMeta.Partitions < 1 |
There was a problem hiding this comment.
Pull request overview
This PR upgrades the golangci-lint tooling from v1.x to v2.x by migrating the configuration file format and updating version references across the codebase. The upgrade also addresses several lint findings that were identified by the new linter version.
Key Changes:
- Migrated
.golangci.ymlfrom v1 to v2 format, reorganizing linters, settings, and exclusions - Updated golangci-lint from v1.55.2/v1.64 to v2.7.2 across scripts and CI workflows
- Addressed lint findings by simplifying redundant code patterns (boolean logic, ObjectMeta field access)
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
.golangci.yml |
Migrated configuration to v2 format with restructured linters, settings, and exclusions sections |
scripts/lint.sh |
Updated golangci-lint installation version from v1.55.2 to v2.7.2 |
.github/workflows/golangci-lint.yml |
Upgraded GitHub action from v6 to v8 and linter version to v2.7.2 |
pkg/admin/impl.go |
Simplified boolean assignment logic per lint suggestion |
controllers/serviceaccountbinding_controller.go |
Simplified ObjectMeta.Name access to direct Name field access |
controllers/secret_controller.go |
Simplified ObjectMeta.DeletionTimestamp access to direct DeletionTimestamp field access |
.gitignore |
Added local development tool files to ignore list |
go.sum |
Added missing go.mod entry for dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - linters: | ||
| - revive | ||
| text: 'exported: exported method .*\.(Reconcile|SetupWithManager|SetupWebhookWithManager) should have comment or be unexported' | ||
| - linters: | ||
| - errcheck | ||
| text: Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked | ||
| - linters: | ||
| - revive | ||
| text: exported (method|function|type|const) (.+) should have comment or be unexported | ||
| source: (func|type).*Fake.* | ||
| - linters: | ||
| - revive | ||
| path: fake_\.go | ||
| text: exported (method|function|type|const) (.+) should have comment or be unexported | ||
| - linters: | ||
| - revive | ||
| path: cmd/clusterctl/internal/test/providers.*.go | ||
| text: exported (method|function|type|const) (.+) should have comment or be unexported | ||
| - linters: | ||
| - revive | ||
| path: (framework|e2e)/.*.go | ||
| text: exported (method|function|type|const) (.+) should have comment or be unexported | ||
| - linters: | ||
| - unparam | ||
| text: always receives | ||
| - path: _test\.go | ||
| text: should not use dot imports | ||
| - path: (framework|e2e)/.*.go | ||
| text: should not use dot imports | ||
| - path: _test\.go | ||
| text: cyclomatic complexity | ||
| - linters: | ||
| - gocritic | ||
| text: 'appendAssign: append result not assigned to the same slice' | ||
| - linters: | ||
| - ifshort | ||
| path: controllers/mdutil/util.go | ||
| text: variable .* is only used in the if-statement | ||
| - linters: | ||
| - staticcheck | ||
| path: .*(api|types)\/.*\/conversion.*\.go$ | ||
| text: 'SA1019: in.(.+) is deprecated' | ||
| - linters: | ||
| - revive | ||
| path: .*(api|types)\/.*\/conversion.*\.go$ | ||
| text: exported (method|function|type|const) (.+) should have comment or be unexported | ||
| - linters: | ||
| - revive | ||
| path: .*(api|types)\/.*\/conversion.*\.go$ | ||
| text: 'var-naming: don''t use underscores in Go names;' | ||
| - linters: | ||
| - revive | ||
| path: .*(api|types)\/.*\/conversion.*\.go$ | ||
| text: 'receiver-naming: receiver name' |
There was a problem hiding this comment.
Multiple exclusion rules reference the revive linter (lines 99, 105, 109, 113, 117, 141, 145, 149), but revive is not in the enabled linters list. These exclusions will have no effect. Either add revive to the enabled linters if these exclusions are needed, or remove these exclusion rules to clean up the configuration.
There was a problem hiding this comment.
@maxsxu or @freeznet Enabling the revive linter adds a lot of the following lint failures:
exported method|type|const ... should have comment or be unexported
and many like
var-naming: struct field SqlScript should be SQLScript
Most of the revive lints seem stylistic, I don't really like them but wanted to ask your thoughts? Do you want it enabled or should I leave it disabled and remove the config associated with it?
.golangci.yml
Outdated
| godot: | ||
| scope: toplevel | ||
| exclude: | ||
| - ^ \+.* | ||
| - ^ ANCHOR.* |
There was a problem hiding this comment.
The godot linter has settings configured but is not enabled in the linters list (line 26). These settings will have no effect. If godot linting is desired, add it to the enabled linters list. Otherwise, consider removing these unused settings to keep the configuration clean.
| godot: | |
| scope: toplevel | |
| exclude: | |
| - ^ \+.* | |
| - ^ ANCHOR.* |
There was a problem hiding this comment.
Removing godot linter config - godot is used for adding "." at the end of sentenences however most of the comments/docs in the repo do not follow this pattern so it doesn't seem like this should be enabled
3 participants
Bumps the golangci-lint config to v2, address some lint findings from new config. Config was migrated with automation - https://golangci-lint.run/docs/product/migration-guide/
Motivation
New versions of golangci-lint don't support the old v1 config file format. Maintaining two versions of golangci-lint locally can be annoying, especially when you are integrating golangci-lint into your editor for lsp 😄
Modifications
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Documentation
no-need-docOnly upgrading tooling separate from the operator itself