make lint return error with Go version 1.16.3

[daehyeok]

Steps to reproduce the issue:

1. After update Go to 1.16.3 make lint always return Error.

❯ env | rg ^GO
GOPATH=/Users/daehyeok/workspace/minikube/_gopath
GOROOT=/Users/daehyeok/.asdf/installs/golang/1.16.3/go
❯ go version
go version go1.16.3 darwin/amd64
❯ make lint
./out/linters/golangci-lint-v1.39.0 run --timeout 7m --build-tags "integration " --enable gofmt,goimports,gocritic,golint,gocyclo,misspell,nakedret,stylecheck,unconvert,unparam,dogsled --exclude 'variable on range scope.*in function literal|ifElseChain' --skip-files "pkg/minikube/translate/translations.go|pkg/minikube/assets/assets.go" ./...
pkg/drivers/kic/oci/oci.go:115:46: SA4023: this comparison is never true (staticcheck)
                if cgroup2, err := IsCgroup2UnifiedMode(); err == nil && cgroup2 {
                                                           ^
pkg/drivers/kic/oci/cgroups_other.go:28:6: SA4023(related information): k8s.io/minikube/pkg/drivers/kic/oci.IsCgroup2UnifiedMode never returns a nil interface value (staticcheck)
func IsCgroup2UnifiedMode() (bool, error) {
     ^
pkg/drivers/kic/oci/oci.go:130:46: SA4023: this comparison is never true (staticcheck)
                if cgroup2, err := IsCgroup2UnifiedMode(); err == nil && cgroup2 {

rule SA4023 looks added on latest version. If i use 1.16.2 make lint don't blame about SA4023

❯ env | rg ^GO
GOPATH=/Users/daehyeok/workspace/minikube/_gopath
GOROOT=/Users/daehyeok/.asdf/installs/golang/1.16.2/go
❯ go version
go version go1.16.2 darwin/amd64
❯ make lint
./out/linters/golangci-lint-v1.39.0 run --timeout 7m --build-tags "integration " --enable gofmt,goimports,gocritic,golint,gocyclo,misspell,nakedret,stylecheck,unconvert,unparam,dogsled --exclude 'variable on range scope.*in function literal|ifElseChain' --skip-files "pkg/minikube/translate/translations.go|pkg/minikube/assets/assets.go" ./...

[afbjorklund]

minikube currently uses 1.16.1

GO_VERSION ?= 1.16.1

But we should update to 1.16.3

https://github.com/kubernetes/kubernetes/blob/master/build/build-image/cross/VERSION

[afbjorklund]

The issue itself should be simple. Care to submit a PR ?

// IsCgroup2UnifiedMode returns whether we are running in cgroup 2 cgroup2 mode.
func IsCgroup2UnifiedMode() (bool, error) {
        return false, errors.Errorf("Not supported on %s", runtime.GOOS)
}

Possibly this should add a bogus if runtime.GOOS != "linux"

We already have that when using it, and in the tag, but the linter seems to be stupid.

[daehyeok]

/assign

sure i will

[daehyeok]

yeah it looks linter issue. 1.16.4 release few hours ago. and this problem gone again.

❯ env | rg ^GO
GOPATH=/Users/daehyeok/workspace/minikube/_gopath
GOROOT=/Users/daehyeok/.asdf/installs/golang/1.16.4/go
❯ go version
go version go1.16.4 darwin/amd64
❯ make lint
./out/linters/golangci-lint-v1.39.0 run --timeout 7m --build-tags "integration " --enable gofmt,goimports,gocritic,golint,gocyclo,misspell,nakedret,stylecheck,unconvert,unparam,dogsled --exclude 'variable on range scope.*in function literal|ifElseChain' --skip-files "pkg/minikube/translate/translations.go|pkg/minikube/assets/assets.go" ./...

[afbjorklund]

Something like

// +build !linux

// IsCgroup2UnifiedMode returns whether we are running in cgroup 2 cgroup2 mode.
func IsCgroup2UnifiedMode() (bool, error) {
    if runtime.GOOS != "linux" {
        return false, errors.Errorf("Not supported on %s", runtime.GOOS)
    } else {
        panic()
    }
}

Or whatever it takes to make that confused "staticcheck" happy

[dominikh]

rule SA4023 looks added on latest version. If i use 1.16.2 make lint don't blame about SA4023

Staticcheck is not part of Go. Upgrading from Go 1.16.2 to 1.16.4 does not introduce new checks to Staticcheck. Upgrading Go may change the behavior of checks, however, especially ones that analyze the behavior of the standard library (which would apply to SA4023).

but the linter seems to be stupid

I'd love to make it "less stupid", but I can't do so if people don't report bugs upstream. I found this issue by chance.

I have tried to reproduce the issue, but no combination of GOOS, Go version and Staticcheck version (either standalone or in golangci-lint) could reproduce this issue, and from looking at your code, I don't see why the warning would be emitted, either.

You can read the check's documentation for what it actually aims to flag. It doesn't merely fire because a function never returns an untyped nil, but only if it also returns typed nils. Neither IsCgroup2UnifiedMode nor errors.Errorf do this, so I don't know what's up.

[afbjorklund]

I'd love to make it "less stupid", but I can't do so if people don't report bugs upstream. I found this issue by chance.

We know that linters are stupid, that's why they are non-blocking. As supposed to other tools, which are enforced.

That is not a major problem per se, sometimes we have to flag/workaround a false positive but that's OK too...

Thanks for taking the time to comment here!

If we can find the issue, will report upstream.

[daehyeok]

I'd love to make it "less stupid", but I can't do so if people don't report bugs upstream. I found this issue by chance.

I didn't report yet, because i couldn't narrow down which component fire the this warning. after little more investigation with my problematic env, i will make issue for it.

P.S thank for the comment and appreciate for the emacs go-mode 👍

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[afbjorklund]

Should be OK now, and minikube is up to go version 1.16.6 (same as Kubernetes 1.22)