-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
update golangci-lint to v1.51.2 #5085
update golangci-lint to v1.51.2 #5085
Conversation
This PR has multiple commits, and the default merge method is: merge. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
.golangci.yml
Outdated
# please, do not use `enable-all`: it's deprecated and will be removed soon. | ||
# inverted configuration with `enable-all` and `disable` is not scalable during updates of golangci-lint |
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.
enable-all
option is Un-deprecated.
golangci/golangci-lint#1888
.golangci.yml
Outdated
- exhaustruct | ||
- deadcode | ||
- scopelint | ||
- nonamedreturns | ||
- golint | ||
- maintidx | ||
- nosnakecase |
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.
These linters were not "comment out" on config, but didn't enabled.
.golangci.yml
Outdated
@@ -6,93 +6,34 @@ run: | |||
go: '1.19' | |||
|
|||
linters: | |||
# please, do not use `enable-all`: it's deprecated and will be removed soon. | |||
# inverted configuration with `enable-all` and `disable` is not scalable during updates of golangci-lint |
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 agree that we can remove the comment about enable-all being deprecated, but I think the second comment inverted configuration with
enable-alland
disable is not scalable during updates of golangci-lint
is still valid. I think it will be better to use disable-all
and enable
, but you can go ahead and enable to the specific linters you want. It will make it easier to review upgrades this way too
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.
Thanks for your comment.
I think the second comment inverted configuration with enable-all and disable is not scalable during updates of golangci-lint is still valid.
Sorry I don't understand why enable-all
is not scalable during updates because we are still pinning golangci-lint
version on Makefile and I think we add new linter with checking golangci-lint
release notes every update PR make.
So, I think the disable-all
and enable
operation is unscalable than the enable-all
and we didn't investigate what we should enable.
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, I will revert it for now.
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 see. Ok, I'll approve this PR right now so that we can get the lint check passing again. And feel free to open a new PR with enable-all
and disable
so we can discuss that separately.
5860615
to
39264a7
Compare
This reverts commit 39264a7.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: koba1t, natasha41575 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Update golangci-lint v1.51.2.