Migrate to golang CI LINT v2 #693
Conversation
WalkthroughThis pull request introduces several updates and refinements across the project. The GitHub Actions workflow for linting Go code has been revised by updating action versions and removing deprecated steps. The Go linting configuration now uses a new version, updated linter settings, and added formatters. Various test files have been modified for improved clarity via variable renaming. Multiple files have been updated to handle resource cleanup using deferred anonymous functions that ignore errors. Additionally, error message formatting has been standardized, a new tool installation check has been added in a development script, and several dependencies have been removed from the tools module. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as dev/up Script
participant System as Operating System
participant Brew as Homebrew
Script->>System: Check for "golangci-lint" using which command
alt Tool Found
System-->>Script: Return tool path
Script->>Script: Continue execution
else Tool Not Found
System-->>Script: Return not found
Script->>Brew: Execute "brew install golangci-lint"
Brew-->>Script: Confirm installation
Script->>Script: Continue execution
end
sequenceDiagram
participant Client as Caller
participant Func as HttpAddressToGrpcTarget
Client->>Func: Pass URL with scheme (https, http, or unknown)
alt Scheme is "https"
Func-->>Client: Return target with isTLS = true
else Scheme is "http" or empty
Func-->>Client: Return target with isTLS = false
else Unknown Scheme
Func-->>Client: Return error "unknown connection schema"
end
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Upgrade golangci-lint to v2 and consolidate linting configuration to improve code quality checksUpdates the Go linting infrastructure and configuration across the codebase:
📍Where to StartStart with the workflow configuration in .github/workflows/lint-go.yml and the linter configuration in .golangci.yaml to understand the core changes to the linting setup. Macroscope summarized 5a86e83. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.golangci.yaml (1)
14-17: Add a newline at the end of the fileThe file is missing a newline at the end, which is a common best practice for YAML files and is flagged by the linter.
- gofmt - golines +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 17-17: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
.github/workflows/lint-go.yml(1 hunks).golangci.yaml(1 hunks)cmd/cli/main.go(1 hunks)dev/up(1 hunks)pkg/api/message/subscribe_test.go(2 hunks)pkg/api/query_test.go(11 hunks)pkg/blockchain/migrator/migrator.go(2 hunks)pkg/db/pgx_test.go(6 hunks)pkg/db/subscription.go(1 hunks)pkg/interceptors/client/auth_test.go(3 hunks)pkg/migrations/migrations.go(2 hunks)pkg/mlsvalidate/service.go(2 hunks)pkg/testutils/network/port.go(1 hunks)pkg/upgrade/docker_utils_test.go(2 hunks)pkg/utils/grpc.go(1 hunks)tools/go.mod(0 hunks)
💤 Files with no reviewable changes (1)
- tools/go.mod
🧰 Additional context used
🧬 Code Definitions (3)
pkg/api/query_test.go (3)
pkg/db/types.go (1)
NullInt32(12-14)pkg/testutils/store.go (2)
CreatePayer(114-127)InsertGatewayEnvelopes(93-112)pkg/testutils/api/api.go (1)
NewTestReplicationAPIClient(212-221)
pkg/api/message/subscribe_test.go (2)
pkg/testutils/api/api.go (1)
NewTestReplicationAPIClient(212-221)pkg/testutils/store.go (1)
CreatePayer(114-127)
pkg/mlsvalidate/service.go (1)
pkg/utils/grpc.go (1)
GetCredentialsForAddress(37-57)
🪛 YAMLlint (1.35.1)
.golangci.yaml
[error] 17-17: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Code Review
- GitHub Check: Lint-Go
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
🔇 Additional comments (31)
pkg/blockchain/migrator/migrator.go (2)
89-91: Good practice for handling close errors.The change to use a deferred anonymous function that ignores errors when closing the file is a good practice. This approach explicitly shows the intention to ignore any potential errors from file closing while ensuring the resource is properly cleaned up.
101-103: Consistent error handling pattern applied.The same deferred closure pattern has been correctly applied to this file operation as well, maintaining consistency throughout the codebase.
cmd/cli/main.go (1)
134-134: Error message formatting standardized.The capitalization change from "Could" to "could" standardizes the error message format to match Go's conventional style of using lowercase at the beginning of error messages.
pkg/db/subscription.go (2)
54-54: Error message capitalization standardized.The change from "Already started" to "already started" standardizes the error message format to follow Go's conventional style of using lowercase at the beginning of error messages.
57-57: Error message capitalization standardized.Similar to the previous change, this standardizes the error message format by using lowercase at the beginning, following Go's error message conventions.
pkg/utils/grpc.go (1)
21-27: Improved control flow with switch statementThe refactoring from if-else to switch statement enhances readability and makes the code structure cleaner. The explicit handling of empty string alongside "http" is a good improvement. The error message formatting change to lowercase aligns with Go style conventions.
pkg/interceptors/client/auth_test.go (3)
82-82: Improved error handling during test cleanupUsing the
_ = listener.Close()pattern properly ignores any potential errors during resource cleanup, which is a good practice in test code.
95-95: Consistent resource cleanup patternThe deferred anonymous function that ignores connection close errors is a good practice in test code, ensuring resources are cleaned up without test failures due to non-critical cleanup issues.
113-115: Consistent approach to connection cleanupUsing a multi-line deferred function closure matches the pattern used elsewhere in the codebase, making error handling consistent across connection cleanup operations.
pkg/api/message/subscribe_test.go (1)
31-31: Improved variable naming for consistencyRenaming
dbtodbHandleacross the function improves code clarity and maintains consistency with the naming patterns used elsewhere in the codebase. This change enhances readability without altering functionality.Also applies to: 33-33, 89-89
pkg/testutils/network/port.go (1)
13-15: Better resource cleanup with ignored errorsConverting the simple
defer ln.Close()to a deferred anonymous function ignoring errors is consistent with the cleanup pattern being adopted throughout the codebase. This prevents potential issues if the Close operation were to fail.pkg/api/query_test.go (3)
25-27: Improved variable naming for better clarity.The parameter rename from
dbtodbHandleanddb_rowstodbRowsimproves code readability by:
- Differentiating the parameter from the package name
db- Following Go naming conventions with camelCase instead of snake_case
79-80: LGTM: Updated variable reference.Consistent use of the renamed variable
dbHandleanddbRowsthroughout the function.
84-86: LGTM: Updated variable names consistently.Variable renaming has been consistently applied when calling
setupQueryTest.pkg/db/pgx_test.go (6)
31-31: Improved resource cleanup error handling.Using an anonymous function with
_ = namespacedDB.Close()properly ignores any errors that might occur during cleanup, which is appropriate for test teardown.
36-38: Enhanced resource management with proper error handling.The deferred anonymous function pattern consistently ignores errors during resource cleanup, which is the recommended approach when these errors shouldn't affect test outcomes.
63-63: Consistent application of error-ignoring cleanup pattern.Using the same pattern for multiple resources shows good consistency in the codebase's approach to resource management.
Also applies to: 76-76
112-114: Appropriate error handling in network resource cleanup.The changes properly handle potential errors from closing network listeners both in the deferred function and the context cancellation handler.
Also applies to: 118-118
133-135: LGTM: Consistent error handling for connection closure.This follows the same pattern used throughout the file for resource cleanup.
148-148: LGTM: Consistent error handling.The change properly ignores any errors from closing the listener, which is appropriate in this test setup.
pkg/migrations/migrations.go (3)
7-7: LGTM: Added errors package import.This import is necessary for the improved error checking with
errors.Is().
22-24: Enhanced resource management with consistent error handling.The pattern of using deferred anonymous functions to ignore errors during cleanup has been consistently applied to all resources in this function. This prevents cleanup errors from affecting the function's primary purpose.
Also applies to: 30-32, 38-40, 46-48
51-51: Improved error checking with errors.Is.Using
errors.Is()instead of direct comparison is a significant improvement as it properly handles wrapped errors, following Go's error handling best practices.pkg/mlsvalidate/service.go (2)
30-30: Improved error message formatting.Changed error message capitalization from "Failed to..." to "failed to..." following Go style conventions where error messages typically start with lowercase unless the first word is a proper noun.
Also applies to: 35-35
53-53: Consistent error handling during connection closure.Using
_ = conn.Close()to explicitly ignore any errors during connection closure follows the same pattern applied throughout the codebase, ensuring consistent resource management..golangci.yaml (2)
1-3: Correctly updated golangci-lint configuration version and approachThe change from version "1" to "2" and from
disable-all: truetodefault: noneproperly aligns with the new golangci-lint v2 configuration style. This is a good update that maintains the same selective linting approach while following the new version's conventions.
11-13: Good addition of lintersAdding
nilerrandnakedretlinters will help catch common Go errors like returning nil instead of a non-nil error and naked returns in long functions, improving code quality.pkg/upgrade/docker_utils_test.go (2)
83-83: Good use of strings.ReplaceAllReplacing
strings.Replace(varValue, "localhost", "host.docker.internal", -1)withstrings.ReplaceAll(varValue, "localhost", "host.docker.internal")improves code readability by using the more explicitReplaceAllfunction instead of using-1as the count parameter inReplace.
139-141: Improved error handling in deferred CloseThe change from
defer reader.Close()to a deferred anonymous function with explicit error ignoring (defer func() { _ = reader.Close() }()) is a good practice. It clearly indicates that you're aware of potential errors from Close but are intentionally ignoring them, making the error handling strategy explicit..github/workflows/lint-go.yml (2)
23-24: Good update to latest GitHub Actions versionsUpdating to actions/checkout@v4 and actions/setup-go@v5 ensures you're using the latest features and security fixes provided by these actions.
28-30: Correctly updated golangci-lint action for v2 supportThe update to golangci-lint-action@v7 with the version parameter set to v2.0 properly configures the workflow to use golang-ci-lint v2. This aligns with the updated .golangci.yaml configuration and simplifies the workflow by removing the need for separate timeout and config path parameters, which are now likely handled by defaults or in the config file itself.
Changes in a67b427
Macroscope summarized a67b427. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.golangci.yaml (1)
18-18: Add Newline at End-of-File.
YAMLlint has flagged a missing newline at the end of the file. Please add a newline character at the end to comply with common formatting best practices.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (69)
.github/workflows/lint-go.yml(1 hunks).golangci.yaml(1 hunks)cmd/cli/main.go(1 hunks)cmd/replication/main.go(1 hunks)dev/up(1 hunks)pkg/api/http_server.go(1 hunks)pkg/api/message/publishWorker.go(0 hunks)pkg/api/message/service.go(1 hunks)pkg/api/message/subscribe_test.go(2 hunks)pkg/api/metadata/cursorUpdater.go(3 hunks)pkg/api/metadata/service.go(1 hunks)pkg/api/payer/clientManager_test.go(0 hunks)pkg/api/payer/nodeCursorTracker.go(2 hunks)pkg/api/payer/nodeCursorTracker_test.go(2 hunks)pkg/api/payer/nodeSelector_test.go(0 hunks)pkg/api/payer/service.go(2 hunks)pkg/api/query_test.go(11 hunks)pkg/api/server.go(1 hunks)pkg/authn/claims.go(2 hunks)pkg/authn/claims_test.go(1 hunks)pkg/authn/signingMethod_test.go(1 hunks)pkg/authn/tokenFactory.go(1 hunks)pkg/authn/tokenFactory_test.go(1 hunks)pkg/authn/verifier.go(1 hunks)pkg/authn/verifier_test.go(1 hunks)pkg/blockchain/blockchainPublisher.go(0 hunks)pkg/blockchain/migrator/migrator.go(2 hunks)pkg/blockchain/nonceManager.go(2 hunks)pkg/blockchain/nonceManager_test.go(1 hunks)pkg/blockchain/ratesAdmin.go(0 hunks)pkg/blockchain/rpcLogStreamer.go(1 hunks)pkg/db/congestion.go(0 hunks)pkg/db/pgx.go(1 hunks)pkg/db/pgx_test.go(7 hunks)pkg/db/queries_test.go(1 hunks)pkg/db/sequences_test.go(2 hunks)pkg/db/subscription.go(1 hunks)pkg/db/types.go(1 hunks)pkg/envelopes/envelopes_test.go(0 hunks)pkg/indexer/blockTracker.go(1 hunks)pkg/indexer/blockTracker_test.go(1 hunks)pkg/indexer/indexer.go(0 hunks)pkg/indexer/storer/identityUpdate.go(0 hunks)pkg/interceptors/client/auth.go(0 hunks)pkg/interceptors/client/auth_test.go(3 hunks)pkg/interceptors/server/auth.go(1 hunks)pkg/interceptors/server/logging.go(1 hunks)pkg/interceptors/server/logging_test.go(2 hunks)pkg/interceptors/server/openConnections.go(1 hunks)pkg/metrics/payer.go(1 hunks)pkg/metrics/sync.go(1 hunks)pkg/migrations/migrations.go(2 hunks)pkg/mlsvalidate/service.go(2 hunks)pkg/mlsvalidate/service_test.go(1 hunks)pkg/payerreport/manager.go(0 hunks)pkg/registry/contractRegistry.go(0 hunks)pkg/registry/node.go(0 hunks)pkg/registry/notifier.go(0 hunks)pkg/server/server.go(0 hunks)pkg/server/server_test.go(3 hunks)pkg/sync/syncWorker.go(2 hunks)pkg/testutils/api/api.go(1 hunks)pkg/testutils/contracts.go(1 hunks)pkg/testutils/network/port.go(1 hunks)pkg/testutils/versioning.go(1 hunks)pkg/upgrade/docker_utils_test.go(2 hunks)pkg/utils/grpc.go(1 hunks)pkg/utils/hash.go(1 hunks)tools/go.mod(0 hunks)
💤 Files with no reviewable changes (16)
- pkg/indexer/storer/identityUpdate.go
- pkg/blockchain/blockchainPublisher.go
- pkg/db/congestion.go
- pkg/registry/contractRegistry.go
- pkg/api/payer/nodeSelector_test.go
- pkg/registry/notifier.go
- pkg/indexer/indexer.go
- pkg/api/payer/clientManager_test.go
- pkg/interceptors/client/auth.go
- pkg/server/server.go
- pkg/registry/node.go
- pkg/blockchain/ratesAdmin.go
- pkg/api/message/publishWorker.go
- pkg/payerreport/manager.go
- pkg/envelopes/envelopes_test.go
- tools/go.mod
✅ Files skipped from review due to trivial changes (38)
- pkg/authn/claims.go
- pkg/db/pgx.go
- pkg/authn/signingMethod_test.go
- pkg/authn/tokenFactory_test.go
- pkg/testutils/api/api.go
- pkg/api/server.go
- pkg/db/sequences_test.go
- pkg/api/http_server.go
- pkg/db/queries_test.go
- pkg/authn/tokenFactory.go
- pkg/interceptors/server/logging.go
- pkg/authn/verifier.go
- pkg/testutils/versioning.go
- pkg/metrics/sync.go
- pkg/blockchain/nonceManager_test.go
- pkg/authn/verifier_test.go
- cmd/replication/main.go
- pkg/authn/claims_test.go
- pkg/utils/hash.go
- pkg/blockchain/rpcLogStreamer.go
- pkg/api/metadata/cursorUpdater.go
- pkg/testutils/contracts.go
- pkg/mlsvalidate/service_test.go
- pkg/interceptors/server/openConnections.go
- pkg/interceptors/server/auth.go
- pkg/interceptors/server/logging_test.go
- pkg/metrics/payer.go
- pkg/api/payer/nodeCursorTracker_test.go
- pkg/api/message/service.go
- pkg/api/payer/nodeCursorTracker.go
- pkg/sync/syncWorker.go
- pkg/indexer/blockTracker_test.go
- pkg/db/types.go
- pkg/api/metadata/service.go
- pkg/indexer/blockTracker.go
- pkg/api/payer/service.go
- pkg/blockchain/nonceManager.go
- pkg/server/server_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/db/subscription.go
- pkg/blockchain/migrator/migrator.go
- pkg/interceptors/client/auth_test.go
- cmd/cli/main.go
- pkg/db/pgx_test.go
- pkg/testutils/network/port.go
- pkg/migrations/migrations.go
- pkg/api/message/subscribe_test.go
- pkg/upgrade/docker_utils_test.go
- pkg/mlsvalidate/service.go
- pkg/utils/grpc.go
- pkg/api/query_test.go
- dev/up
- .github/workflows/lint-go.yml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.golangci.yaml
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (4)
.golangci.yaml (4)
1-1: Update to Configuration Version.
The version has been updated to"2", which aligns with the requirements of the new golangci-lint configuration.
3-3: Modified Linters Default Setting.
The change from usingdisable-all: truetodefault: nonemakes the configuration more explicit by only enabling the linters listed.
11-12: Addition of Nil Error and Naked Return Checks.
Including- nilerrand- nakedretin the enabled linters list is a good move to improve error handling and code readability. Please verify these additions align with your project’s coding standards.
14-18: Introduction of Formatters Section.
The newformatterssection, which enablesgofmt,golines, andgofumpt, is a welcome addition for enforcing code style consistency. This helps maintain uniform formatting across the codebase.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
Supercedes #683
Summary by CodeRabbit
Chores
golangci-linttool is available before proceeding.Style
Tests
These behind-the-scenes improvements deliver a more robust, consistent, and reliable product experience for end users.