Should we enable more linting?

[justinsb]

Inspired by #775 I was wondering why the linter didn't catch the error (or warn us about it). Turns out checking for unhandled errors does not have consensus, and is thus not implemented in go vet.

But we could turn on golangci-lint - other k8s subprojects do use it.

Here's the current output with the default configuration:

git-sync  (master) > golangci-lint run
main.go:480:34: Error return value of `pflag.CommandLine.MarkDeprecated` is not checked (errcheck)
        pflag.CommandLine.MarkDeprecated("branch", "use --ref instead")
                                        ^
main.go:483:34: Error return value of `pflag.CommandLine.MarkDeprecated` is not checked (errcheck)
        pflag.CommandLine.MarkDeprecated("change-permissions", "use --group-write instead")
                                        ^
main.go:486:34: Error return value of `pflag.CommandLine.MarkDeprecated` is not checked (errcheck)
        pflag.CommandLine.MarkDeprecated("dest", "use --link instead")
                                        ^
main.go:1726:14: Error return value is not checked (errcheck)
        refreshCreds(ctx)
                    ^
main.go:1895:16: Error return value of `io.WriteString` is not checked (errcheck)
        io.WriteString(h, s)
                      ^
main_test.go:750:7: Error return value is not checked (errcheck)
        touch(dirPath)
             ^
main_test.go:759:7: Error return value is not checked (errcheck)
        touch(filePath)
             ^
main.go:1933:13: S1039: unnecessary use of fmt.Sprintf (gosimple)
                sshCmd += fmt.Sprintf(" -o StrictHostKeyChecking=no")
                          ^
main.go:2087:3: S1023: redundant `return` statement (gosimple)
                return
                ^
main.go:1152:33: SA1016: os.Kill cannot be trapped (did you mean syscall.SIGTERM?) (staticcheck)
        signal.Notify(c, os.Interrupt, os.Kill)

[justinsb]

Looking at the list the only one I think is important is the one @karlkfi identified (refreshCreds). The touch errcheck warning is test code. The io.WriteString is writing to a hasher which I believe is documented to never fail.

This is just the default set of linters, there could well be others in the not-enabled-by-default linters. If there are any recent bugs (that a linter could have caught) I can see if they would have been caught.

[thockin]

In short, yes, I'd be happy with more lint-free code.

The signal one seems like a mistake, and touch is suspect to me.