Conversation
Signed-off-by: Anisur Rahman <anisur@appscode.com>
There was a problem hiding this comment.
Pull request overview
This PR upgrades the project to use golangci-lint 2.x with automated code modernization and linting fixes.
- Adds new
.golangci.ymlconfiguration file for golangci-lint 2.x - Updates Go version in
go.modand simplifies Makefile linting command - Applies automated code modernization:
interface{}→any, removal of unused imports, API simplifications, and idiomatic improvements
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.golangci.yml |
New configuration file for golangci-lint 2.x with linter settings and formatters |
Makefile |
Simplified golangci-lint command to use configuration file |
go.mod |
Updated Go version requirement |
test/e2e/**/*.go |
Added // nolint: staticcheck for dot imports of testing libraries |
test/e2e/matcher/*.go |
Modernized matcher functions with any type and error message improvements |
test/e2e/framework/*.go |
Modernized type signatures, removed unused imports, simplified API calls |
pkg/**/*.go |
Multiple modernizations: interface{} → any, removed unused imports, simplified API access patterns, improved string operations |
pkg/volumesnapshot/*.go |
Updated timestamp comparisons and test client API access |
hack/gendocs/main.go |
Updated string replacement calls to use ReplaceAll |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,27 @@ | |||
| version: "2" | |||
| linters: | |||
| default: standard | |||
There was a problem hiding this comment.
The linters.default configuration value "standard" is not a valid option in golangci-lint. The valid options for enabling linters are typically "enable", "disable", "enable-all", or "disable-all". This will likely cause configuration errors.
| default: standard | |
| default: enable |
| return matcher.find(obj) | ||
| default: | ||
| return false, fmt.Errorf("Unknown object type") | ||
| return false, fmt.Errorf("unknown object type") |
There was a problem hiding this comment.
The error message "unknown object type" is less descriptive than the original "Unknown object type". While lowercase is technically more idiomatic for Go error messages, the capitalized version was clearer. Consider making the message more descriptive, such as "unsupported object type" or including the actual type received.
| return matcher.find(obj) | ||
| default: | ||
| return false, fmt.Errorf("Unknown object type") | ||
| return false, fmt.Errorf("unknown object type") |
There was a problem hiding this comment.
The error message "unknown object type" is less descriptive than the original "Unknown object type". Consider making the message more descriptive, such as "unsupported object type" or including the actual type received for better debugging.
|
|
||
| func (vs VolumeSnapshots) Less(i, j int) bool { | ||
| return vs[i].VolumeSnap.CreationTimestamp.Time.After(vs[j].VolumeSnap.CreationTimestamp.Time) | ||
| return vs[i].VolumeSnap.CreationTimestamp.After(vs[j].VolumeSnap.CreationTimestamp.Time) |
There was a problem hiding this comment.
The comparison has been changed from comparing two Time values to comparing a metav1.Time with a time.Time. The original code vs[i].VolumeSnap.CreationTimestamp.Time.After(vs[j].VolumeSnap.CreationTimestamp.Time) properly extracted both time.Time values for comparison. The new code vs[i].VolumeSnap.CreationTimestamp.After(vs[j].VolumeSnap.CreationTimestamp.Time) calls the After method on metav1.Time, which expects a time.Time parameter. While metav1.Time does have an After method that accepts time.Time, this inconsistency should be verified for correctness. Both sides should consistently use either .Time or rely on metav1.Time's After method.
| return vs[i].VolumeSnap.CreationTimestamp.After(vs[j].VolumeSnap.CreationTimestamp.Time) | |
| return vs[i].VolumeSnap.CreationTimestamp.Time.After(vs[j].VolumeSnap.CreationTimestamp.Time) |
| } | ||
| vsClient := vsfake.NewSimpleClientset(volumeSnasphots...) | ||
| vsClient.Fake.Resources = []*metav1.APIResourceList{ | ||
| vsClient.Resources = []*metav1.APIResourceList{ |
There was a problem hiding this comment.
The change from vsClient.Fake.Resources to vsClient.Resources removes the explicit .Fake accessor. While this may work if the fake client directly exposes the Resources field, this change should be verified against the external-snapshotter client library version being used. The .Fake field is a common pattern in Kubernetes fake clients to access the underlying fake implementation's fields.
| vsClient.Resources = []*metav1.APIResourceList{ | |
| vsClient.Fake.Resources = []*metav1.APIResourceList{ |
| formatters: | ||
| enable: | ||
| - gofmt | ||
| - goimports | ||
| settings: | ||
| gofmt: | ||
| rewrite-rules: | ||
| - pattern: 'interface{}' | ||
| replacement: 'any' | ||
|
|
There was a problem hiding this comment.
The configuration file specifies version "2" which corresponds to golangci-lint 2.x. However, the structure and syntax used in this file don't match the actual golangci-lint v2.x configuration format. The formatters section and its structure do not exist in golangci-lint. The correct structure should use linters-settings for configuring individual linters. This configuration may not work as intended.
| formatters: | |
| enable: | |
| - gofmt | |
| - goimports | |
| settings: | |
| gofmt: | |
| rewrite-rules: | |
| - pattern: 'interface{}' | |
| replacement: 'any' |
Signed-off-by: Tamal Saha <tamal@appscode.com>
2 participants
No description provided.