chore: upgrade golangci-lint to v2 and modernize codebase #68
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request upgrades golangci-lint from v1.64.8 to v2.7.2 and modernizes the codebase to comply with updated linting requirements. The changes include configuration updates, dependency upgrades, and widespread code improvements focused on documentation, error handling, and code quality.
Changes:
- Upgraded golangci-lint to v2.7.2 with updated configuration format
- Migrated from deprecated JWT library (dgrijalva/jwt-go) to maintained fork (golang-jwt/jwt)
- Added comprehensive package and function documentation comments throughout the codebase
- Improved error handling patterns with explicit error ignoring in defer statements
- Enhanced file security with stricter permissions and filepath.Clean() usage
Reviewed changes
Copilot reviewed 64 out of 65 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| .golangci.yml | Updated configuration to v2 format with new linter and output settings |
| .github/workflows/lint.yaml | Updated linter version to 2.7.2 and simplified output format flag |
| go.mod, go.sum | Migrated JWT dependency to maintained golang-jwt/jwt library |
| pkg/schema/*.go | Added function documentation and improved code formatting |
| pkg/mcp/**/*.go | Added comprehensive documentation, improved error handling, and refactored code |
| pkg/config/*.go | Added documentation, improved error handling, and tightened file permissions |
| pkg/auth/**/*.go | Migrated to new JWT library and improved error handling |
| pkg/kafka/*.go | Added documentation and improved defer error handling |
| pkg/pulsar/*.go | Added documentation and improved code structure |
| pkg/cmd/mcp/*.go | Added documentation, improved imports, and tightened file security |
| pkg/common/utils.go | Added documentation comments |
| pkg/log/io.go | Added package documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if _, err := os.Stat(o.ConfigDir); os.IsNotExist(err) { | ||
| err := os.MkdirAll(o.ConfigDir, 0755) | ||
| err := os.MkdirAll(o.ConfigDir, 0750) |
There was a problem hiding this comment.
The file permission mode has been changed from 0755 to 0750. This is more restrictive and secure (removing execute permission for others), but it may cause issues if the config directory needs to be readable by other users or processes. Verify that this change doesn't break any use cases where the directory needs broader access permissions.
| err := os.MkdirAll(o.ConfigDir, 0750) | |
| err := os.MkdirAll(o.ConfigDir, 0755) |
| } | ||
|
|
||
| fd, err := os.OpenFile(filePath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0666) | ||
| fd, err := os.OpenFile(filepath.Clean(filePath), os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) |
There was a problem hiding this comment.
The log file permission has been changed from 0666 to 0600, which is more secure by restricting read/write access to the owner only. However, verify this doesn't break any operational scenarios where the log file needs to be read by other users or log aggregation tools running under different accounts.
| fd, err := os.OpenFile(filepath.Clean(filePath), os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) | |
| fd, err := os.OpenFile(filepath.Clean(filePath), os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644) |
| formatters: | ||
| enable: | ||
| - gofmt | ||
| - goimports | ||
|
|
||
| output: | ||
| formats: colored-line-number | ||
| print-issued-lines: true | ||
| print-linter-name: true | ||
| formats: | ||
| text: | ||
| path: stdout | ||
| print-issued-lines: true | ||
| print-linter-name: true |
There was a problem hiding this comment.
The golangci-lint v2 configuration format has changed significantly from v1. The 'formatters' section is not a valid top-level key in v2. Additionally, gofmt and goimports are not formatters but linters in golangci-lint. The 'output.formats' structure also appears incorrect for v2 - it should be 'output.format' (singular). Please verify this configuration against the golangci-lint v2 documentation and consider testing it locally before merging, as it may cause the linter to fail or ignore these settings.
|
|
||
| package common | ||
| // Package common provides shared helpers for StreamNative MCP Server. | ||
| package common //nolint:revive |
There was a problem hiding this comment.
The nolint directive on the package declaration may not be necessary. The 'common' package name doesn't shadow any standard library package and is a widely accepted generic package name. Unless revive is configured with a specific rule against generic package names like 'common', this suppression may be unnecessary and could be removed.
| package common //nolint:revive | |
| package common |
No description provided.