Bump golangci-lint and enable modernize check #6992
Conversation
Signed-off-by: RainbowMango <[email protected]>
Signed-off-by: RainbowMango <[email protected]>
karmada-bot
added
do-not-merge/hold
karmada-bot
requested review from
CharlesQQ,
Garrybest,
Poor12,
Vacant2333,
XiShanYongYe-Chang,
calvin0327,
chaosi-zju,
chaunceyjiang,
huntsman-li,
ikaven1024,
jwcesign,
lonelyCZ,
mrlihanbo,
mszacillo,
whitewindmills,
wuyingjun-lucky and
zhzhuang-zju
karmada-bot
added
the
size/XXL
Summary of ChangesHello @RainbowMango, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing code quality and maintainability by upgrading the project's linter infrastructure. It introduces a newer version of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request bumps golangci-lint and enables the modernize linter, which has resulted in a large number of small, mechanical changes across the codebase to adopt more modern Go features. These changes include using any instead of interface{}, leveraging the slices and maps packages, using strings.Builder for performance, and adopting Go 1.21+ features like min/max builtins and Go 1.22 features like for range on integers. The changes are generally positive and improve code quality and performance. The configuration changes in .golangci.yml to disable some checks are well-justified for a large, existing codebase. I've found one minor issue in a test file where a refactoring made the test logic less clear, but overall, this is a great modernization effort.
| for range rsl { | ||
| if reflect.TypeFor[corev1.ResourceName]().String() != "v1.ResourceName" { | ||
| t.Errorf("Got %v expected %v", reflect.TypeFor[corev1.ResourceName](), "v1.ResourceName") | ||
| } | ||
| } |
There was a problem hiding this comment.
The loop variable name was removed, but it was used in the original code inside reflect.TypeOf(name). The new code for range rsl with reflect.TypeFor[corev1.ResourceName]() inside the loop makes the loop redundant as it checks the same static type information in every iteration. This change seems to be a result of a faulty linter suggestion and makes the test less clear. Please consider restoring the use of the loop variable or refactor the test to perform the check once outside the loop if that's the intent.
There was a problem hiding this comment.
Pull request overview
This PR modernizes the codebase by bumping golangci-lint to v2.6+ and applying the modernize linter to simplify code using newer Go language features. The changes are primarily automated refactorings that leverage Go 1.21+ features including:
- Replacing
interface{}withanyalias - Using
slices.Contains()instead of manual loops - Using
maps.Copy()for map copying - Using range-over-integer syntax (
for range n) - Using
strings.SplitSeq()iterator-based splitting - Using
fmt.Appendf()instead offmt.Sprintf()with byte slices - Using
min()builtin function - Using
reflect.TypeFor[T]()instead ofreflect.TypeOf((*T)(nil)) - Using
t.Context()in tests instead of manual context creation - Using
b.Loop()in benchmarks
Reviewed changes
Copilot reviewed 221 out of 221 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/helper/scheduler.go | Replaced manual contains loops with slices.Contains |
| test/helper/resource.go | Updated interface{} to any, for loop to range syntax |
| test/e2e/suites/operator/api_sidecar_test.go | Replaced loop with slices.Contains |
| test/e2e/suites/base/*.go | Updated map[string]interface{} to map[string]any throughout test files |
| test/e2e/framework/*.go | Updated function signatures to use any instead of interface{} |
| pkg/webhook/**/*.go | Updated interface{} to any, added slices usage |
| pkg/util/*.go | Added maps/slices imports, modernized loops and map copying |
| pkg/scheduler/**/*.go | Updated interface{} to any, modernized for loops |
| pkg/resourceinterpreter/**/*.go | Comprehensive updates to test fixtures using any |
| pkg/search/**/*.go | Updated handlers and context usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6992 +/- ##
==========================================
- Coverage 46.61% 46.55% -0.06%
==========================================
Files 699 699
Lines 48156 48078 -78
==========================================
- Hits 22447 22385 -62
+ Misses 24014 24002 -12
+ Partials 1695 1691 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dongjiang1989 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR bumps golangci-lint to v2.6+ so that we can use modernize to simplify the code.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This PR contains a workaround commit to ignore the false positive warning as tracked by golangci/golangci-lint#5979. We can wait for that to be resolved before moving forward.
/hold
Does this PR introduce a user-facing change?: