Use golangci-lint #111
Use golangci-lint #111
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
|
Ideally, I would prefer to get rid of this legacy file in favor of golangci-lint, which is the tool that we are using in all the other subprojects. Do you think you could do the migration in this PR? |
|
Ok, I also prefer golangci-lint a lot. I'll update the PR (and the prometheus-adapter's one) to use it. |
k8s-ci-robot
added
size/L
|
PR updated with golangci-lint. Most changes are just formatting / style / variables renaming (or adding
|
There was a problem hiding this comment.
The project really needed it 😆, thank you for looking into that @olivierlemasle
.golangci.yml
Outdated
| issues: | ||
| exclude-use-default: true | ||
| exclude-rules: | ||
| # We don't check duplicate code in the tests. |
There was a problem hiding this comment.
This is odd, why do we need it?
There was a problem hiding this comment.
You're right, I should remove this exclusion.
It was for duplicated code between handleCustomMetrics and handleExternalMetrics; but I could refactor that.
Makefile
Outdated
| # Format and lint | ||
| # --------------- | ||
|
|
||
| GOLANGCI_VERSION:=1.50.1 |
There was a problem hiding this comment.
nit: this would be more convenient to have at the top-level for the future since it would allow having all the versions of the tools defined in one block
pkg/apiserver/apiserver.go
Outdated
| @@ -41,7 +41,7 @@ func init() { | |||
| eminstall.Install(Scheme) | |||
|
|
|||
| // we need custom conversion functions to list resources with options | |||
| installer.RegisterConversions(Scheme) | |||
| installer.RegisterConversions(Scheme) //nolint: errcheck | |||
There was a problem hiding this comment.
Let's add a todo here to fix this errcheck, not checking errors is pretty bad.
More generally, this piece of code shouldn't be in a init() function, also it installs both custom-metrics and external-metrics every time even though some projects might only want to import one API.
There was a problem hiding this comment.
Ok. I added a nolint exception because I thought it was the same thing that was all over Kubernetes with these zz_generated.conversion.go files (e.g. here), but with a closer look, it's not.
I agree that this code needs to be reworked because custom-metrics and external-metrics should not be both statically installed unconditionally.
There was a problem hiding this comment.
Oh correct, I didn't check what RegisterConversions was doing when commenting but now that you've explained that is written the same way as zz_generated.conversion.go generated by the code-generator from k8s, I am fine with keeping the exception.
There was a problem hiding this comment.
I switched to k8s.io/apimachinery/pkg/util/runtime.Must(), which is used in this kind of cases in Kubernetes. It would panic the same way in case of error, but at least it is not linter-specific.
|
@dgrisonnet can we please release a new version once this PR is merged? So we can consume k8s 0.25 deps? Thanks! |
|
Sure |
|
Thanks @olivierlemasle! /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgrisonnet, olivierlemasle, zroubalik 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 |
k8s-ci-robot
added
the
approved
Original PR:
Switch to golangci-lint