Suggestion: Implement Go linting

[sargun]

Brief summary including motivation/context:

This is more of a meta-issue. I'm curious as to whether there's any interest in adding a linter to the project. I tested golangci-lint, and it runs in 1.685 seconds. I used the following config:

linters:
  enable:
    - golint
    - gosec
    - interfacer
    - unconvert
    - dupl
    - goconst
    - gocyclo
    - goimports
    - misspell
    - scopelint
    - gofmt

It showed the following output. Although most of these lint identifications aren't super important, some of them might catch places where the code could be simpler, or where the code might be ambiguous.

config/config_test.go:164:32: Using the variable on range scope `testCase` in function literal (scopelint)
            actual, err := New([]string{testCase.env})
                                        ^
config/config_test.go:170:18: Using the variable on range scope `testCase` in function literal (scopelint)
            if process != testCase.wantProcess {
                          ^
config/config_test.go:171:42: Using the variable on range scope `testCase` in function literal (scopelint)
                t.Errorf("Want process %v, got: %v", testCase.wantProcess, process)
                                                     ^
executor/afterburn_runner.go:96:10: Error return value of `w.Write` is not checked (errcheck)
        w.Write(bodyBytes)
               ^
executor/http_runner.go:94:21: Error return value of `cmd.Process.Signal` is not checked (errcheck)
        cmd.Process.Signal(syscall.SIGTERM)
                          ^
executor/http_runner.go:187:10: Error return value of `w.Write` is not checked (errcheck)
        w.Write(bodyBytes)
               ^
executor/serializing_fork_runner.go:26:10: Error return value of `w.Write` is not checked (errcheck)
        w.Write([]byte(err.Error()))
               ^
main.go:168:23: Error return value of `functionInvoker.Start` is not checked (errcheck)
    functionInvoker.Start()
                         ^
main.go:298:23: Error return value of `functionInvoker.Start` is not checked (errcheck)
    functionInvoker.Start()
                         ^
config/config.go:130:2: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (gosimple)
    if env[key] == "true" {
    ^
executor/http_runner.go:155:3: should use a simple channel send/receive instead of select with a single case (gosimple)
        select {
        ^
main.go:31:26: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
        fmt.Fprintf(os.Stderr, configErr.Error())
                               ^
main.go:108:5: should omit comparison to bool constant, can be simplified to !suppressLock (gosimple)
    if suppressLock == false {
       ^
main.go:129:3: redundant break statement (gosimple)
        break
        ^
main.go:132:3: redundant break statement (gosimple)
        break
        ^
main.go:135:3: redundant break statement (gosimple)
        break
        ^
main.go:258:2: should merge variable declaration with assignment on next line (gosimple)
    var envs []string
    ^
main.go:335:55: should omit comparison to bool constant, can be simplified to !lockFilePresent() (gosimple)
            if atomic.LoadInt32(&acceptingConnections) == 0 || lockFilePresent() == false {

Any design changes

In the Dockerfile where we run the tests, we would have to install the linter, and run the linters.

Pros + Cons

Pros

• It makes it easier to avoid "dumb" code issues
• It simplifies the code

  Cons

• It takes time to run
• Sometimes linters can be annoying

Effort required up front

It would require that we take the current repo, fix, or ignore, the linter issues, and then add a linter configuration, as well as configuration to invoke the linter at CI time.

Effort required for CI/CD, release, ongoing maintenance

Basically keeping up to date the linter, and the linter configuration.

Migration strategy / backwards-compatibility

See the effort required up front.

[alexellis]

Derek add label: proposal

[alexellis]

@sargun thanks for writing up your proposal and for covering the points suggested in the contribution guide :+1:

Linting and static code analysis is definitely something very widely used with OOP languages like Java and C# (sometimes with the Python community and PEPs), but I haven't encountered this much in the Go community. This is the first request for linting since the project started in 2016.

Cons: Sometimes linters can be annoying

This question is placed in the wrong repo - if linting is to be introduced and followed / honoured then it needs to be part of the overall contribution process for the whole project.

At this time I do not see linting as offering strong benefits but I may change my mind in the future if I see enough value for the cost to implement across the whole project. That said I tend to use a less strict linting implicitly through my IDE when making patches.

[sargun]

I've seen linting used in the following projects to enforce Code "policy":

• Docker: https://github.com/moby/moby/blob/master/hack/validate/gometalinter
• Virtual-kubelet: https://github.com/virtual-kubelet/virtual-kubelet/blob/master/.circleci/config.yml#L12-L17
• mesos-dns: https://github.com/mesosphere/mesos-dns/blob/master/circle.yml#L35
• Kubernetes: https://github.com/kubernetes/repo-infra/blob/master/verify/go-tools/verify-gometalinter.sh

It's not so much of a request, as more of a codification of the practices of the project.

[mrwormhole]

well lucky me, if this is still in progress, might help with linter rules?

I use this for projects and I have scraped countless linter rules from golangci-lint configs because sometimes some rules are the same but maintained by 2 different projects. We can always customize linter settings but I prefer one config for one org that everyone agrees, it is super easy to customize but metalinters are painful to customize so we use defaults of these rulesets (gocritic/revive/govet/staticcheck)

https://gist.github.com/MrWormHole/250a8a1f78ca17c059b8ce075bbc28ec

I can see from @sargun 's list

    - golint (this is no longer valid, superceeded by revive of Grafana labs)
    - gosec (valid)
    - interfacer (deprecated no longer a good practice)
    - unconvert (valid)
    - dupl (valid but requires a config to ignore test files)
    - goconst (valid)
    - gocyclo (valid)
    - goimports (valid)
    - misspell (valid but UK and US folks fight for this a lot, we use UK setting for the language, I don't like this linter)
    - scopelint (absolute dead and no longer used, superceeded by [exportloopref](https://github.com/kyoh86/exportloopref))
    - gofmt (valid)