chore(golangci-lint): v1.54.2 pinning and config file #256
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
There was a problem hiding this comment.
@prometherion - thanks for this PR.
Added some inline comments.
.github/workflows/test.yaml
Outdated
| @@ -50,7 +50,8 @@ jobs: | |||
| - name: golangci-lint | |||
| uses: golangci/golangci-lint-action@v3 | |||
| with: # TODO: remove this when the deprecated function will be removed (issue #85) | |||
| args: --exclude SA1019 --timeout=5m -E ginkgolinter | |||
| args: --timeout=5m -v | |||
| version: v1.44.0 | |||
There was a problem hiding this comment.
The latest version of the golangci-lint is v1.54.1 - any reason not to use it?
There was a problem hiding this comment.
As stated in the previous comment section, the idea was to install the golangci-lint binary using go install to avoid interferences from global packages.
However, the latest releases are requiring Go +v1.19 and the go.mod is referencing 1.18. We can decouple from the local development environment from the build one, it should be then well documented without giving anything for granted.
There was a problem hiding this comment.
1.18 is deprecated anyway. Maybe we better upgrade to 1.20 or 1.21?
There was a problem hiding this comment.
Alternatively, we can download the binary
There was a problem hiding this comment.
So @prometherion, I'm working on a new PR to bump golang, here: #257
It's now failing on the unit test, but this is as result of changes in #255. I'll try to fix it next week. But anyway, I think we need to find ways to use newer version of the golangci-lint. v1.44 is way too old.
There was a problem hiding this comment.
Glad I introduced a feature and a bug at the same time, story of my life.
If you need help with this, please, assign it to me, I'm happy to contribute to the project.
Once we bump to 1.20 we can easily install using the go-install-tool helper: downloading the binary could be cumbersome and require more kinds of checks such as architecture, os, etc.
There was a problem hiding this comment.
I plan to look at it next week. But if you'll have some time, here is the unit test failure log:
------------------------------
•! [PANICKED] [0.001 seconds]
Reconcile
/home/nunnatsa/GIT/cluster-api-provider-kubevirt/controllers/kubevirtcluster_controller_test.go:37
reconcile cluster with finalizer and deletion time stamp
/home/nunnatsa/GIT/cluster-api-provider-kubevirt/controllers/kubevirtcluster_controller_test.go:142
[It] should succeed with the kubevirt cluster being deleted.
/home/nunnatsa/GIT/cluster-api-provider-kubevirt/controllers/kubevirtcluster_controller_test.go:154
Test Panicked
In [It] at: /usr/local/go1.20.3/src/runtime/panic.go:260
runtime error: invalid memory address or nil pointer dereference
Full Stack Trace
sigs.k8s.io/cluster-api-provider-kubevirt/controllers.(*KubevirtClusterReconciler).deleteExtraGVK(0x37b3ce0, 0xc000707100, {{0x2614d0d, 0x0}, {0x2614d0e, 0x2}, {0x2622de1, 0xd}})
/home/nunnatsa/GIT/cluster-api-provider-kubevirt/controllers/kubevirtcluster_controller.go:292 +0x323
sigs.k8s.io/cluster-api-provider-kubevirt/controllers.(*KubevirtClusterReconciler).reconcileDelete(0x37b3ce0, 0xc000707100, 0x7f37fced24a8?)
/home/nunnatsa/GIT/cluster-api-provider-kubevirt/controllers/kubevirtcluster_controller.go:255 +0x5ec
sigs.k8s.io/cluster-api-provider-kubevirt/controllers.(*KubevirtClusterReconciler).Reconcile(0x37b3ce0, {0x28db298?, 0xc00004c120}, {{{0x0?, 0x37e4790?}, {0x2631233?, 0x37e4790?}}})
/home/nunnatsa/GIT/cluster-api-provider-kubevirt/controllers/kubevirtcluster_controller.go:155 +0xaaa
sigs.k8s.io/cluster-api-provider-kubevirt/controllers_test.glob..func1.5.3()
/home/nunnatsa/GIT/cluster-api-provider-kubevirt/controllers/kubevirtcluster_controller_test.go:167 +0x1ea
------------------------------
What we'll need to do is to mock the new KubevirtClusterReconciler.APIReader.
|
@prometherion - #257 was just merged, bumping golang to version 1.20 Could you please try again with the latest version of golangci-lint? |
k8s-ci-robot
added
the
needs-rebase
|
/retest |
prometherion
force-pushed
the
chore/declarative-golangci-lint
branch
from
86beaa4
to
f4be09f
Compare
k8s-ci-robot
removed
the
needs-rebase
k8s-ci-robot
added
size/M
prometherion
force-pushed
the
chore/declarative-golangci-lint
branch
3 times, most recently
from
d0c41fd
to
093335c
Compare
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
|
/hold Need to run ci @davidvossel I think there is a button for admins to allow ci to run. Could you please check? |
k8s-ci-robot
added
the
do-not-merge/hold
|
@prometherion, could you please update PR title with the right version of golangci-lint? |
@agradouski @cchengleo - could you do that? |
prometherion
changed the title
prometherion
force-pushed
the
chore/declarative-golangci-lint
branch
from
093335c
to
d9065fa
Compare
|
/lgtm @prometherion - the setting of this repo require one-time approval from an admin. This is why the github actions are not running. The funny thing is that it's is possible to merge even without it... Let's wait for one of the admins to approve you as a contributor. |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nunnatsa, prometherion, qinqon 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
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
What this PR does / why we need it:
Currently, there's no version pinning for the
golangci-lintutility used, I had to manually retrieve it.Rather, having it declared, makes it easy for contributors to linting the code.
A local binary will be stored in the
./binfolder to avoid collision with global installed Go modules.Which issue this PR fixes:
Untracked
Special notes for your reviewer:
Release notes: