-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
bump golangci-lint to v1.53.3 #18931
Conversation
Disable new linters and drop comments on them. Signed-off-by: Valentin Rothberg <[email protected]>
Helpful reports to avoid unnecessary allocations. Signed-off-by: Valentin Rothberg <[email protected]>
Because we shouldn't waste assigns. Signed-off-by: Valentin Rothberg <[email protected]>
It turns out, after iterating over rows, we need to check for errors. It also turns out that we did not do that at all. Signed-off-by: Valentin Rothberg <[email protected]>
To make sure the e2e tests are kept in order. Signed-off-by: Valentin Rothberg <[email protected]>
But disable the `unused-parameter` linter as there are just too many reports that I could handle. Also allow unused nolintlint reports. Signed-off-by: Valentin Rothberg <[email protected]>
@Luap99 re-enabled and partially disabled ✔️ |
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, two comments. And they really show how dangerous ginkgo test are because the test can be so easily written incorrectly and thus turn into a NOP.
Which revealed that absent --authfile's are ignored but shouldn't. The issue is now being tracked in containers#18938. Signed-off-by: Valentin Rothberg <[email protected]>
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
Let's make sure to always specify the expected exit codes, even in case of failure. Signed-off-by: Valentin Rothberg <[email protected]>
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
/hold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, Luap99, vrothberg 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 |
/hold cancel |
Initially disable new linter (warnings). Also fix some straight away as the warnings concerned me, especially the sql error checks. Please refer to the individual commits for details.
Does this PR introduce a user-facing change?