-
Notifications
You must be signed in to change notification settings - Fork 1.7k
✨ (go/v4): upgrade golangci-lint to v2.12.1 #5676
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 6 commits
aa4414e
29a7c08
4aa1086
e4fedc5
f38779e
88aa6e6
e264b9b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,9 @@ linters: | |
| logcheck: | ||
| type: "module" | ||
| description: Checks Go logging calls for Kubernetes logging conventions. | ||
| goconst: | ||
| ignore-tests: true | ||
| min-occurrences: 5 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the goal of this config is to reduce noise from test files and we are already ignoring the tests, is
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm...so the summary in the description is a bit misleading. In regards to this config, I favor explicitly adding no-lint directives to ocurrences than silently ignoring them. Also, a minimum of 5 seems a bit too much, doesn't it? It may solve problems at hand, but might also hide ocurrences in the long run.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Agreed on both points. • I’ll lower min-occurrences to 3. • For specific cases, we’ll use explicit //nolint directives instead of silent ignores WDYT? Default value
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Final offer: min of 2, take it or leave it! Just kidding. Let's wait and hear what @camilamacedo86 has to say.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not think that we should ignore
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. I'll remove
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am concern with the changes in the default scaffold. |
||
| revive: | ||
| rules: | ||
| - name: comment-spacings | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,9 @@ linters: | |
| logcheck: | ||
| type: "module" | ||
| description: Checks Go logging calls for Kubernetes logging conventions. | ||
| goconst: | ||
| ignore-tests: true | ||
| min-occurrences: 5 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not think we should add it at all and change the golang ci config for the default scaffold, could we revert this change?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmmm.... The lint rules in samples are broader in scope than those in golangci-linter default value(remove it). 🤔
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is in the sample because the default scaffold was changed. |
||
| revive: | ||
| rules: | ||
| - name: comment-spacings | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we should add it here as well.
What are the failures ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove it done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so many errors: