ci: migrate golangci lint to v2 #20658
Conversation
|
Hi @LJTian. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
|
Can one of the admins verify this patch? |
|
/lint |
|
/ok-to-test |
|
thank you @LJTian for picking this up ! are the Excludes in this PR new ? are they easy to fix in a follow up PR? |
Makefile
Outdated
| @@ -525,7 +524,7 @@ out/linters/golangci-lint-$(GOLINT_VERSION): | |||
| ifeq ($(MINIKUBE_BUILD_IN_DOCKER),y) | |||
| lint: | |||
| docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:$(GOLINT_VERSION) \ | |||
| golangci-lint run ${GOLINT_OPTIONS} --skip-dirs "cmd/drivers/kvm|cmd/drivers/hyperkit|pkg/drivers/kvm|pkg/drivers/hyperkit" ./... | |||
| golangci-lint run ${GOLINT_OPTIONS} --config .golangci-build-in-docker.yaml ./... | |||
There was a problem hiding this comment.
why do we have a different file for build in docker? can we use the same file for simplicity ?
There was a problem hiding this comment.
Since version 2 removed parameters like --skip-dirs , the same functionality can only be achieved through configuration files.
I personally prefer to use a single configuration file to reduce maintenance costs, and I'd like to spend some time to investigate further to see if there is a better way to solve this problem.
There was a problem hiding this comment.
What is the Difference bwtween the current two config files ? can we at least comment What is the diff in the top of the file if there is only one Line difference, we can comment so the next person editing one edit the other one too
There was a problem hiding this comment.
The main difference between these two configuration files is that some directories are skipped
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| disabled: true | ||
| gocritic: | ||
| # See https://go-critic.com/overview.html | ||
| disabled-checks: |
There was a problem hiding this comment.
are these skipped in v1 too ? can we create an issue to fix all these Skipped Lint Issues as a follow up PR
There was a problem hiding this comment.
old version info :
# options for lint (golangci-lint)
GOLINT_OPTIONS = --timeout 7m \
--build-tags "${MINIKUBE_INTEGRATION_BUILD_TAGS}" \
--enable gofmt,goimports,gocritic,revive,gocyclo,misspell,nakedret,stylecheck,unconvert,unparam,dogsled \
--exclude 'variable on range scope.*in function literal|ifElseChain'In our previous makfile, we mainly used regular expressions to skip some issues. In fact, we should use configuration to disable them instead of regular expressions.
We can add a new PR to fix the issues we think need to be fixed. What we need to know is that these fixes are only code style fixes, not code errors or code vulnerabilities.
Makefile
Outdated
| golangci-lint run ${GOLINT_OPTIONS} --skip-dirs "cmd/drivers/kvm|cmd/drivers/hyperkit|pkg/drivers/kvm|pkg/drivers/hyperkit" ./... | ||
| mkdir -p ./out/linters | ||
| @if [ ! -f ./out/linters/yq ] ; then \ | ||
| wget https://github.com/mikefarah/yq/releases/download/${YQ_VERSION}/${YQ_BINARY_NAME} -O ./out/linters/yq && chmod +x ./out/linters/yq; \ |
There was a problem hiding this comment.
this doesnt seem right to install things using wget, and we we need to install this ?
There was a problem hiding this comment.
Currently this code is still under debugging. I submit this PR mainly for the convenience of discussion. I will merge and submit it later. Then notify you again.
Makefile
Outdated
| -i .golangci.yaml && echo "yq add data ok" | ||
|
|
||
| docker run --rm -v `pwd`:/app -w /app golangci/golangci-lint:$(GOLINT_VERSION) \ | ||
| bash -c "apt update && apt -y install libvirt-dev && golangci-lint run ${GOLINT_OPTIONS} ./..." |
There was a problem hiding this comment.
did we really need to do apt update and install libvirt ?
There was a problem hiding this comment.
Currently this code is still under debugging. I submit this PR mainly for the convenience of discussion. I will merge and submit it later. Then notify you again.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LJTian, medyagh 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 |
Currently this code is still under debugging. I submit this PR mainly for the convenience of discussion. I will merge and submit it later. Then notify you again. |
I use the vscode space environment to verify the "build in docker" method, but it always times out. I am still looking for a new environment for verification. After the verification is completed, I will add corresponding comments and remove some redundant information. |
|
New changes are detected. LGTM label has been removed. |
|
/cc @medyagh Hello, I have merged all the commits and this is the latest version. |
This comment has been minimized.
This comment has been minimized.
if our yaml format needs to be standardized so the yq doesnt mess it up, it is okay to change our yaml format let me know when you are satisfied with the PR to be Ready for Review ! feel free to ping me on public minikube chat room on slack |
|
kvm2 driver with docker runtime Times for minikube ingress: 19.7s 18.5s 15.6s 18.6s 19.1s Times for minikube start: 53.4s 50.8s 52.0s 51.5s 55.0s docker driver with docker runtime Times for minikube start: 26.4s 26.4s 24.7s 26.7s 26.5s Times for minikube ingress: 12.8s 12.3s 12.4s 12.3s 13.4s docker driver with containerd runtime Times for minikube start: 22.9s 22.6s 25.6s 23.2s 22.8s Times for minikube ingress: 38.8s 22.8s 38.8s 22.8s 38.9s |
Update golint from v1.64.5 to v2.1.2
add .golangci.yaml file
Fixes #20654