-
Notifications
You must be signed in to change notification settings - Fork 1.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
⚠️ Refactor golangci-lint config to remove false negatives #4657
⚠️ Refactor golangci-lint config to remove false negatives #4657
Conversation
79b2ae8
to
beed942
Compare
|
beed942
to
d46e0cf
Compare
/retest |
/test pull-cluster-api-test-main |
That's definitely a big PR, but I'm glad we are doing this now vs later in the cycle |
@fabriziopandini @CecileRobertMichon Could you folks double check the incompatible changes from above? |
.golangci.yml
Outdated
@@ -23,11 +23,10 @@ linters: | |||
- nolintlint | |||
- prealloc | |||
- rowserrcheck | |||
- scopelint | |||
- exportloopref |
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 think the linters are sorted alphabetically
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.
Do we have a linter for that linter 🙃 ? I got no errors locally, but I'd rather not enforce something that cannot be tested/validated
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.
Nope, but I guess not every review comment can be enforced by linter and the list would be nicer to navigate if it stays sorted :) (although it's btw possible to integrate custom linters into golangci-lint :))
P.S. But if you want to keep it as, it's fine for me.
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.
Fixed it, although we should either automate that or just keep it random
Apart from the two nits => lgtm |
@vincepri I double checked incompatible changes and all of them seems reasonable to me |
During some audit of golangci-lint, it was found that one of the exclude directives was matching a lot more than it should have. The removal of the unwanted regex caused a large backlog of linting errors to show up. This changeset brings in a refactor in the golangci-lint configuration to make sure we catch more errors in the linting process and enable more linters over time. Signed-off-by: Vince Prignano <[email protected]>
d46e0cf
to
af33e43
Compare
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.
/lgtm
@vincepri ex > er but close enough :) /lgtm |
@vincepri: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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 |
What this PR does / why we need it:
During some audit of golangci-lint, it was found that one of the exclude
directives was matching a lot more than it should have.
The removal of the unwanted regex caused a large backlog of linting
errors to show up. This changeset brings in a refactor in the
golangci-lint configuration to make sure we catch more errors in the
linting process and enable more linters over time.
Marking this as an⚠️ breaking change given that in some cases we're unexporting some variables that are only used internally.
/milestone v0.4
/kind release-blocking