Add staticcheck to verify-golangci

[hlee95]

What type of PR is this?

/kind bug

What this PR does / why we need it:

1. Adds staticcheck to hack/verify-golangci-lint.sh
2. Fixes or ignores staticcheck failures
3. Removes hack/verify-staticcheck.sh and cleans up hack/tools/go.mod

Which issue(s) this PR fixes:

Fixes #103721

Special notes for your reviewer:

There were a lot of failures for SA5011 "possible nil pointer dereference". The original issue showed 3 occurrences, but after I fixed those with //nolint:staticcheck, I found that more showed up the next time I ran the lint check. All of the occurrences of this failure happened on something of the following form:

		rs, err := rc.clientSet.AppsV1().ReplicaSets(rc.nsName).Get(context.TODO(), rc.name, metav1.GetOptions{})
		framework.ExpectNoError(err)
		if rs == nil {
			framework.Failf(rsIsNil)
		}
		return int(rs.Status.ReadyReplicas)

The linter says that:

test/e2e/framework/autoscaling/autoscaling_utils.go:340:17: SA5011: possible nil pointer dereference (staticcheck)
		return int(rs.Status.ReadyReplicas)
		              ^
test/e2e/framework/autoscaling/autoscaling_utils.go:337:6: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
		if rs == nil {
		   ^

I read here that the linter is unable to detect that our framework.Failf() function results in a panic further down the call chain, which causes the linter to flag all of these cases as a violation of SA5011. The proposed solution from that link is to add a panic("unreachable") to framework.Failf() so that the linter will understand that a panic occurs, so I did that and it fixed all of the errors except for two. Those two used Skipf instead of Failf, so I added the panic("unreachable") to Skipf as well. The other option I thought of was to add -e SA5011 in hack/verify-golangci-lint.sh but that seemed less ideal because it could hide real errors.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Welcome @hlee95!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @hlee95. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

[hlee95]

/assign nikhita

[dims]

/ok-to-test

[hlee95]

I moved to separate commits - @hlee95 do you want to steal these commits?

thockin@6931cba thockin@64a6770

Yup I cherry-picked those.

The second does NOT move all the options from the staticcheck script or delete it - I assume you can cover that?

I copied over the checks and ignore_pattern options from the old script; please let me know if I did it correctly and if I missed anything.

[hlee95]

/retest

[BenTheElder]

unrelated to this PR I think but https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/103723/pull-kubernetes-verify/1458620020010520576 does not capture useful results in the junit, only the go mod downloads (which ... are honestly noise ...), I think the golangci-lint errors are not winding up in stdout instead of stderr, so to find them you have to expand the whole build log and read through it.

we should fix that

[aojea]

it's a bit ironic 🙃

In ./hack/verify-golangci-lint.sh line 48:
if [[ "$#" > 0 ]]; then
           ^-- SC2071: > is for string comparisons. Use -gt instead.


In ./hack/verify-golangci-lint.sh line 53:
        pushd ./vendor/k8s.io/$(basename "$d") >/dev/null
                              ^--------------^ SC2046: Quote this to prevent word splitting.

[thockin]

Is this a thing? I don't see it in https://golangci-lint.run/usage/configuration/

[hlee95]

It is indeed not a thing, I will remove it.

[thockin]

@aojea comments fixed in thockin@395bcb8

[fedebongio]

/triage accepted
/remove-sig api-machinery

[thockin]

I found another bug in script - testing fix now. Basically errexit terminates if any of the runs fails, and doesn't run the rest.

[thockin]

thockin@5758c48

[aojea]

/remove priority-backlog
/priority important-soon

Once this is in we have to spend some time getting this right, is easier to prevent than to find needless in haystacks

[aojea]

why the linter doesn't catch these "capture variable on loop" things ?
#106373

[thockin]

Code freeze looming - anything we can do to help make this work?

[BenTheElder]

@aojea #104835 will cover those.

[aojea]

can we justify to get this during the test-freeze period? I think is legit

[k8s-ci-robot]

@hlee95: PR needs rebase.

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.

[aojea]

trying to get this to the finish line #106448
keeping all the author on the commits of course, great job here

[k8s-ci-robot]

@hlee95: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce-ubuntu-containerd	69b6c28	link	true	/test pull-kubernetes-e2e-gce-ubuntu-containerd
pull-kubernetes-node-e2e-containerd	69b6c28	link	true	/test pull-kubernetes-node-e2e-containerd
pull-kubernetes-e2e-gce-csi-serial	69b6c28	link	false	/test pull-kubernetes-e2e-gce-csi-serial
pull-kubernetes-e2e-gce-storage-snapshot	69b6c28	link	false	/test pull-kubernetes-e2e-gce-storage-snapshot
pull-kubernetes-verify-govet-levee	69b6c28	link	true	/test pull-kubernetes-verify-govet-levee
pull-kubernetes-e2e-kind	69b6c28	link	true	/test pull-kubernetes-e2e-kind
pull-kubernetes-typecheck	69b6c28	link	true	/test pull-kubernetes-typecheck
pull-kubernetes-unit	69b6c28	link	true	/test pull-kubernetes-unit
pull-kubernetes-e2e-gce-100-performance	69b6c28	link	true	/test pull-kubernetes-e2e-gce-100-performance
pull-kubernetes-integration	69b6c28	link	true	/test pull-kubernetes-integration
pull-kubernetes-verify	69b6c28	link	true	/test pull-kubernetes-verify
pull-kubernetes-dependencies	69b6c28	link	true	/test pull-kubernetes-dependencies
pull-kubernetes-conformance-kind-ga-only-parallel	69b6c28	link	true	/test pull-kubernetes-conformance-kind-ga-only-parallel
pull-kubernetes-e2e-kind-ipv6	69b6c28	link	true	/test pull-kubernetes-e2e-kind-ipv6
pull-kubernetes-e2e-gce-storage-slow	69b6c28	link	false	/test pull-kubernetes-e2e-gce-storage-slow
pull-kubernetes-conformance-kind-ipv6-parallel	69b6c28	link	false	/test pull-kubernetes-conformance-kind-ipv6-parallel

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.

[thockin]

#106448 supercedes.