Replace golint for golangci-lint

[rikatz]

golint is deprecated and frozen.

We should replace our CI for https://github.com/golangci/golangci-lint, check the flakes (and the real complains) and fix those

/kind bug
/triage accepted
/good-first-issue
/help
/priority important-soon

[k8s-ci-robot]

@rikatz:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

golint is deprecated and frozen.

We should replace our CI for https://github.com/golangci/golangci-lint, check the flakes (and the real complains) and fix those

/kind bug
/triage accepted
/good-first-issue
/help
/priority important-soon

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.

[longwuyuan]

/assign I would like to work on this

[longwuyuan]

/assign

[tao12345666333]

great

We can use https://github.com/golangci/golangci-lint-action

[rikatz]

uhum, but first as Long is studying go I want him to run lint locally, and check and fix the errors :D so he can learn a bit on why the errors happens and what's the fix.

Then we move to use golangci-lint-action and remove the prow job :D

[longwuyuan]

First run ;

~]                                                                                                                                                                                                                                                                       
% curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.49.0                                                                                                                                        
golangci/golangci-lint info checking GitHub for tag 'v1.49.0'                                                                                                                                                                                                             
golangci/golangci-lint info found version: 1.49.0 for v1.49.0/linux/amd64                                                                                                                                                                                                 
golangci/golangci-lint info installed /home/me/go/bin/golangci-lint                                                                                                                                                                                                       
[~]                                                                                                                                                                                                                                                                       
% golangci-lint --version                                                                                                                                                                                                                                                 
golangci-lint has version 1.49.0 built from cc2d97f3 on 2022-08-24T10:24:37Z                                                                                                                                                                                              
[~]                                                                                                                                                                                                                                                                       
% cd Documents/github/longwuyuan/ingress-nginx                                                                                                                                                                                                                            
[~/Documents/github/longwuyuan/ingress-nginx] main                                                                                                                                                                                                                        
% golangci-lint run                                                                                                                                                                                                                                                       
pkg/tcpproxy/tcp.go:120:10: Error return value of `io.Copy` is not checked (errcheck)                                                                                                                                                                                     
                io.Copy(s, c)                                                                                                                                                                                                                                             
                       ^                                                                                                                                                                                                                                                  
pkg/util/process/sigterm_test.go:47:14: Error return value of `syscall.Kill` is not checked (errcheck)                                                                                                                                                                    
        syscall.Kill(syscall.Getpid(), syscall.SIGTERM)                                                                                                                                                                                                                   
                    ^                                                                                                                                                                                                                                                     
pkg/util/file/file_test.go:38:10: Error return value of `f.Write` is not checked (errcheck)                                                                                                                                                                               
                f.Write(test.content)                                                                                                                                                                                                                                     
                       ^                                                                                                                                                                                                                                                  
pkg/util/file/file_test.go:39:9: Error return value of `f.Sync` is not checked (errcheck)                                                                                                                                                                                 
                f.Sync()                                                                                                                                                                                                                                                  
                      ^                                                                                                                                                                                                                                                   
pkg/util/file/file_watcher_test.go:62:14: Error return value of `os.WriteFile` is not checked (errcheck)                                                                                                                                                                  
        os.WriteFile(f.Name(), []byte{}, ReadWriteByUser)                                                                                                                                                                                                                 
                    ^                                                                                                                                                                                                                                                     
internal/net/dns/dns_test.go:43:14: Error return value of `os.WriteFile` is not checked (errcheck)                                                                                                                                                                        
        os.WriteFile(f.Name(), []byte(`                                                                                                                                                                                                                                   
                    ^                                                                                                                                                                                                                                                     
images/fastcgi-helloserver/rootfs/main.go:29:12: Error return value of `fcgi.Serve` is not checked (errcheck)                                                                                                                                                             
        fcgi.Serve(l, nil)                                                                                                                                                                                                                                                
                  ^                                                                                                                                                                                                                                                       
internal/admission/controller/main_test.go:105:21: Error return value of `adm.HandleAdmission` is not checked (errcheck)                                                                                                                                                  
        adm.HandleAdmission(review)                                                                                                                                                                                                                                       
                           ^                                                                                                                                                                                                                                              
internal/admission/controller/main_test.go:115:21: Error return value of `adm.HandleAdmission` is not checked (errcheck)                                                                                                                                                  
        adm.HandleAdmission(review)                                                                                                                                                                                                                                       
                           ^                                                                                                                                                                                                                                              
internal/admission/controller/server.go:34:25: Error return value is not checked (errcheck)                                                                                                                                                                               
        admissionv1.AddToScheme(scheme)                                                                                                                                                                                                                                   
                               ^                                                                                                                                                                                                                                          
cmd/plugin/kubectl/kubectl.go:80:12: Error return value of `io.Copy` is not checked (errcheck)                                                                                                                                                                            
        go io.Copy(writer, op)                                                                                                                                                                                                                                            
                  ^                                                                                                                                                                                                                                                       
cmd/plugin/commands/certs/certs.go:49:24: Error return value of `cobra.MarkFlagRequired` is not checked (errcheck)                                                                                                                                                        
        cobra.MarkFlagRequired(cmd.Flags(), "host")                                                                                                                                                                                                                       
                              ^                                                                                                                                                                                                                                           
test/e2e/settings/proxy_protocol.go:66:13: Error return value of `conn.Write` is not checked (errcheck)                                                                                                                                                                   
                conn.Write([]byte(header))                                                                                                                                                                                                                                
                          ^                                                                                                                                                                                                                                               
test/e2e/settings/proxy_protocol.go:67:13: Error return value of `conn.Write` is not checked (errcheck)                                                                                                                                                                   
                conn.Write([]byte("GET / HTTP/1.1\r\nHost: proxy-protocol\r\n\r\n"))                                                                                                                                                                                      
                          ^                                                                                                                                                                                                                                               
test/e2e/settings/proxy_protocol.go:99:13: Error return value of `conn.Write` is not checked (errcheck)                                                                                                                                                                   
                conn.Write([]byte(header))                                                                                                                                                                                                                                
                          ^                                                                               

and lots more

[rikatz]

nice. Now, there are some warnings that are valid and some that are not :) the majority here are non checked errors (like if err != nil) from a function.

• What happens in some of those errors if they are not checked? (eg can it panic? is it fine not to check?)
• Are there errors not related to "Error return value"? Are those valid?
  Based on that, maybe you can submit some bug fixes on this?

==== Ricardo's personal opinion ====

I am one of those persons that likes to check the return of all errors. I'm a coward when we are talking about errors, always wondering that some non checked error or some non checked pointer may end up in a panic in a future :)

But this is not a common sense (I guess...) and some people thinks that some errors can be ignored. The notable example, IIRC is the io.Close thing, that says that the error can be ignored as usually it is always nil.

This is why I say it should make sense to ignore or not the error (look at the called function, when it can return an error or not) and if it does make sense to ignore that error, we should add the marker in the code ignoring golangci-lint in that part of the code :)

[longwuyuan]

This is going to be very focussed work for me because of me not being a go dev.
The very first errorcheck linter's complain about "return value is not checked" seems to come from a bug in the linter itself, that is still open since the year 2013 (i could be utterly wrong too) . This kisielk/errcheck#127 points at kisielk/errcheck#7 .

If I have got it wrong, I would love to get advise/comments .

[longwuyuan]

@rikatz @tao12345666333 The demo video showed something interesting so I tried it here. Can you comment if the below check is valid or not ;

[~/Documents/github/longwuyuan/ingress-nginx] golangci-lint
% golangci-lint run --new-from-rev controller-v1.3.1
[~/Documents/github/longwuyuan/ingress-nginx] golangci-lint
% golangci-lint run --new-from-rev controller-v1.3.0
pkg/util/process/sigterm_test.go:47:14: Error return value of `syscall.Kill` is not checked (errcheck)
        syscall.Kill(syscall.Getpid(), syscall.SIGTERM)
                    ^
pkg/util/file/file_watcher_test.go:62:14: Error return value of `os.WriteFile` is not checked (errcheck)
        os.WriteFile(f.Name(), []byte{}, ReadWriteByUser)
                    ^
test/e2e/framework/httpexpect/chain.go:40:17: func `(*chain).reset` is unused (unused)
func (c *chain) reset() {
                ^
test/e2e/framework/httpexpect/chain.go:50:17: func `(*chain).assertOK` is unused (unused)
func (c *chain) assertOK(r Reporter) {
                ^
test/e2e/framework/httpexpect/chain.go:44:17: func `(*chain).assertFailed` is unused (unused)
func (c *chain) assertFailed(r Reporter) {

[~/Documents/github/longwuyuan/ingress-nginx] golangci-lint
% 

Only these few since v1.3.0 and none since v1.3.1 .... Does this seem valid ?

I mean I can try to check the last 3 about httpexpect. But these 3 seem to make some sense because I think that relates to a very very recent PR from amim. But they too got cleaned up in controller v1.3.1

[longwuyuan]

Ah, interesting FAQ. Helpful and clear on what to do ;

**Q. How to integrate golangci-lint into large project with thousands of issues

We are sure that every project can easily integrate golangci-lint, even the large one. The idea is to not fix all existing issues. Fix only newly added issue: issues in new code. To do this setup CI (or better use GolangCI) to run golangci-lint with option --new-from-rev=HEAD1. Also, take a look at option --new, but consider that CI scripts that generate unstaged files will make --new only point out issues in those files and not in the last commit. In that regard --new-from-rev=HEAD1 is safer. By doing this you won't create new issues in your code and can choose fix existing issues (or not).**

https://golangci-lint.run/usage/faq/

[longwuyuan]

/remove-help

[longwuyuan]

@rikatz @tao12345666333 this is ready for your comments because using the flag to only check tags after recent release shows no linting error

[tao12345666333]

Thanks, let me take a look

[x448]

% curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.49.0                                                                                                                                        

Executing scripts hosted outside your repo without verification opens door to incidents like Codecov Bash Supply Chain Attack.

Shameless plug: for a safer way to use golangci-lint, you can use use my self-contained workflow.yml (safer-golangci-lint) to download, verify SHA-256, and execute golangci-lint.

It's easy-to-audit and use. It's used by fxamacker/cbor for over a year to make sure CI behavior doesn't change unless maintainers update the workflow or linter settings in their own repo.

Cheers!

[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

[k8s-triage-robot]

This issue is labeled with priority/important-soon but has not been updated in over 90 days, and should be re-triaged.
Important-soon issues must be staffed and worked on either currently, or very soon, ideally in time for the next release.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Deprioritize it with /priority important-longterm or /priority backlog
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[rikatz]

/close

This is fixed!! 🥳

[k8s-ci-robot]

@rikatz: Closing this issue.

In response to this:

/close

This is fixed!! 🥳

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.