Add golangci-lint

[sagikazarmark]

This PR adds golangci-lint to the project.

I disabled all linters that produced an error for now, if you like the direction, I'll fix violations as well.

For now, I added a Makefile to make using the linter easy both locally and in CI.

Alternatively, we could use a GitHub action in CI: https://github.com/actions-contrib/golangci-lint

[peterbourgon]

This seems very complex. Is it that much better than e.g. staticcheck?

[sagikazarmark]

staticcheck is one linter, included in golangci-lint. GolangCI is a set of linters.

The list of disabled linters shouldn't scare you: once the lint violations are fixed, it should become much shorter.

[ChrisHines]

I am not a fan of throwing every known linter at projects. I am a fan of staticcheck. My vote would be for vet and staticcheck in CI.

[sagikazarmark]

What about goimports, gofmt and golint? (I know that at least goimports rules are usually asked users to abide.)

I agree that enabling all linters does not necessarily make sense, but there are tons of others (unparam, unconvert, whitespace, etc) that helps keeping the code clean.

[sagikazarmark]

These are the ones that I would fix and enable:

• unused
• staticcheck
• misspell
• unparam
• unconvert
• structcheck
• golint
• goimports
• gofmt
• ineffassign
• deadcode
• whitespace
• stylecheck
• varcheck

[ChrisHines]

I believe there is a lot of redundant checks in the union of all the things those linters check for. Unused, gosimple, and stylecheck were merged into staticcheck a while ago. I am reasonably sure that staticcheck has checks for the same things as ineffassign, varcheck, and deadcode. Staticcheck also checks for many of the same things as golint.

I am dubious of the value of structcheck.

There is some debate about whether using gofmt and goimports as linters. Others have had problems with doing it.

[peterbourgon]

After some thought, I think I agree with @ChrisHines — I'm +1 on running go vet, golint, staticcheck, and gofmt as linters — but -1 on all of these. Can we make the necessary changes in this PR?

[sagikazarmark]

Sure!

[sagikazarmark]

Looks like golangci has a relatively old version of staticcheck, so it won't run unused OOTB.

Violations found by unused

transport/httprp/server.go:19:2: field `errorEncoder` is unused (unused)
metrics/influxstatsd/influxstatsd.go:211:6: func `last` is unused (unused)
sd/zk/instancer.go:93:21: func `(*Instancer).state` is unused (unused)
metrics/cloudwatch/cloudwatch.go:274:6: func `last` is unused (unused)
sd/zk/util_test.go:20:2: var `e` is unused (unused)
metrics/cloudwatch/cloudwatch.go:51:23: func `(*CloudWatch).apply` is unused (unused)
metrics/dogstatsd/dogstatsd.go:212:6: func `last` is unused (unused)
cmd/kitgen/ast_helpers.go:172:6: func `blockStmt` is unused (unused)
cmd/kitgen/parsevisitor.go:39:3: field `node` is unused (unused)
cmd/kitgen/parsevisitor.go:20:3: field `node` is unused (unused)
cmd/kitgen/templates/full.go:33:6: func `makeExampleEndpoint` is unused (unused)
transport/grpc/client.go:18:2: field `serviceName` is unused (unused)
cmd/kitgen/transform.go:160:6: func `spewDecls` is unused (unused)
sd/dnssrv/instancer_test.go:98:6: type `nopCloser` is unused (unused)
sd/eureka/instancer.go:76:21: func `(*Instancer).getInstances` is unused (unused)
cmd/kitgen/parsevisitor.go:27:3: field `node` is unused (unused)

Might be because of the outdated version, but stylecheck, varcheck and deadcode also find violations, that staticcheck doesn't:

Violations found by stylecheck

auth/casbin/middleware.go:40:30: ST1005: error strings should not be capitalized (stylecheck)
        ErrUnauthorized = errors.New("Unauthorized Access")
                                    ^
transport/amqp/publisher.go:13:7: ST1003: const maxCorrelationIdLength should be maxCorrelationIDLength (stylecheck)
const maxCorrelationIdLength = 255
      ^
transport/http/client_test.go:275:2: ST1003: var testHttpClient should be testHTTPClient (stylecheck)
        testHttpClient := httpClientFunc(func(req *http.Request) (*http.Response, error) {
        ^

Violations found by varcheck

sd/zk/util_test.go:20:2: `e` is unused (varcheck)
        e      = func(context.Context, interface{}) (interface{}, error) { return struct{}{}, nil }
        ^

Violations found by deadcode

metrics/dogstatsd/dogstatsd.go:212:6: `last` is unused (deadcode)
func last(a []float64) float64 {
     ^
metrics/influxstatsd/influxstatsd.go:211:6: `last` is unused (deadcode)
func last(a []float64) float64 {
     ^
sd/zk/util_test.go:20:2: `e` is unused (deadcode)
        e      = func(context.Context, interface{}) (interface{}, error) { return struct{}{}, nil }
        ^
cmd/kitgen/ast_helpers.go:172:6: `blockStmt` is unused (deadcode)
func blockStmt(stmts ...ast.Stmt) *ast.BlockStmt {
     ^
cmd/kitgen/transform.go:160:6: `spewDecls` is unused (deadcode)
func spewDecls(f *ast.File) {
     ^
cmd/kitgen/templates/full.go:33:6: `makeExampleEndpoint` is unused (deadcode)
func makeExampleEndpoint(f ExampleService) endpoint.Endpoint {
     ^
metrics/cloudwatch/cloudwatch.go:274:6: `last` is unused (deadcode)
func last(a []float64) float64 {
     ^

Some other linters you voted against and I don't really understand why you wouldn't want them:

• misspell (don't want to have a check for spelling errors?)
• unconvert (don't want to have a check for unnecessary conversions?)
• goimports (there are files violating the go-kit code style for imports)
• whitespace (debatable, but why not have a check for properly separating the code?)

Violations found by other linters

cmd/kitgen/path_test.go:26:27: `ther` is a misspelling of `there` (misspell)
                testcase("c:\\gopath;\\other", "c:\\gopath\\src\\somewhere", "somewhere")
                                        ^
cmd/kitgen/path_test.go:27:18: `ther` is a misspelling of `there` (misspell)
                testcase("c:\\other;c:\\gopath\\", "c:\\gopath\\src\\somewhere", "somewhere")
                               ^
transport/awslambda/handler_test.go:276:30: unnecessary conversion (unconvert)
        err := json.Unmarshal([]byte(req), &apigwReq)
                                    ^
metrics/prometheus/prometheus_test.go:186:47: unnecessary conversion (unconvert)
                if delta := math.Abs(float64(want) - float64(have)); (delta / float64(want)) > tolerance {
                                                            ^
transport/nats/publisher_test.go:85:44: unnecessary conversion (unconvert)
                        msg.Data = []byte(strings.ToUpper(string(testdata)))
                                                                ^
log/logrus/logrus_logger.go:9: File is not `goimports`-ed with -local github.com/go-kit/kit (goimports)
        "github.com/go-kit/kit/log"
log/log_test.go:9: File is not `goimports`-ed with -local github.com/go-kit/kit (goimports)
        "github.com/go-kit/kit/log"
log/logfmt_logger_test.go:9: File is not `goimports`-ed with -local github.com/go-kit/kit (goimports)
        "github.com/go-kit/kit/log"
util/conn/manager.go:147: unnecessary trailing newline (whitespace)

}
sd/lb/random_test.go:51: unnecessary trailing newline (whitespace)

}
cmd/kitgen/replacewalk.go:80: unnecessary leading newline (whitespace)
        switch n := node.(type) {

cmd/kitgen/transform.go:67: unnecessary trailing newline (whitespace)

}
transport/amqp/subscriber.go:250: unnecessary leading newline (whitespace)
) {

[sagikazarmark]

Not sure what to do about the breaking test: I fixed a lint violation (t.Fatal called in a gorouting), and it consistently breaks. I believe it was never functional in the first place.

What should I do with it?

[ChrisHines]

Looks like golangci has a relatively old version of staticcheck, so it won't run unused OOTB.

Right, so let's not use golangci and use the original tools directly.

[ldez]

Currently golangci-lint uses the latest version of staticcheck

• https://github.com/golangci/golangci-lint/blob/140d51b052f1384094b65097d56105aca1a0591d/go.mod#L63
• https://github.com/dominikh/go-tools/releases

[sagikazarmark]

Closing as this is not the path we are going down. Continuing the work in #1107