Introduce go linting to CI process

[zendern]

• What are you trying to do?

The issue that spurred PR #59 was found via running a go linter. https://github.com/golangci/golangci-lint to be specific. But we should get this or at a dead minimum go vet in place as part of our CI pipeline. This will help our code suck less.

• What feature or behavior is this required for?
  Doing linting on all PRs and master builds

• How could we solve this issue? (Not knowing is okay!)
  Introduce golangci-lint or go vet as part of the build process.

• Anything else?
  golangci-lint has a lot of configurability as well as lots more linters/tools that come with that have the ability to help find and fix issues. Whereas go vet is a do one thing and only that thing. It did point out the same issue fixed as part of Avoid copying a lock by value, which is invalid. #59 but that was it.

golangci-lint does in fact run go vet as part of its process.

The defaults for golangci-lint are as follows :

@flimzy mentioned that he likes to configure some others that are not enabled by default so hopefully they will fill us in on those details and we can discuss further.

I currently dont have strong opinions on what we enable at the moment. At this point getting either in place or even a different option is better than the nothing we have today.

cc @bhamail / @DarthHater / @flimzy

[zendern]

As part of this we will have to triage/fix what is bad in the existing codebase as well since we will want the builds to fail if the linting tool comes back with any issues.

[flimzy]

IMHO, the minimum responsible configuration for go linting is govet and errcheck. These routinely find real bugs in code I work on.

gofmt is also almost a no-brainier to include (even though the gofmt authors say not to run it in CI, since it can change between Go versions--this has yet to be a serious problem for me, though).

For reference, in case anyone cares, this is the standard config I use on my projects, which only reflects my personal opinion on what matters, and should in no way be considered a universal ideal :)

[output]
format = "colored-line-number"

[linters]
enable = [
    "interfacer", "gocyclo", "unconvert", "goimports", "unused", "varcheck",
    "vetshadow", "misspell", "nakedret", "errcheck", "golint", "ineffassign",
    "deadcode", "goconst", "vet", "unparam", "gofmt"
]

[zendern]

@flimzy thanks for sharing. After looking through the list of linters/tools available that looks good to me. Two things I think I would add or want to do.

1. Expand on your list to add the rest of the default enabled linters from golang-ci docs just to be explicit about it.

    format = "colored-line-number"

[linters]
    enable = [
      "interfacer", "gocyclo", "unconvert", "goimports", "unused", "varcheck",
      "vetshadow", "misspell", "nakedret", "errcheck", "golint", "ineffassign",
      "deadcode", "goconst", "vet", "unparam", "gofmt", "gosimple", "staticcheck",
      "structcheck", "typecheck"
    ]

2. When we hook it up in CI to not use the --fix option. I think fixing the issues should be on the dev. We can use that flag locally and have the tool do it but I'd rather be intentional about it instead of having CI just check-in fixes.

[flimzy]

Both good points. I built my list with an old version of the tool. I expect there are new defaults since then, and I should probably add them to my own config.

And for the --fix, I completely agree. I wouldn't trust it to be done automatically. I've also never actually seen it be useful locally, but only a small subset of linters support it anyway.

[flimzy]

A slight tangent, I wonder if it would be worth adding nancy to golangci-lint, using their custom linter instructions.

[zendern]

Re: adding Nancy to golangci-lint.... I’m not sure it feels like the right place to add it. I’d love to add it in to potentially get more usage and keep more security issues out of production but really that tool of tools is more geared towards code level issues and Nancy is more of a dependency level tool.

[flimzy]

A valid point. Although there is one other dep-related linter in golangci-lint already:

depguard: Go linter that checks if package imports are in a list of acceptable packages [fast: true, auto-fix: false]

Anyway, it's not really my place to say what should be done here. The thought just crossed my mind, so I thought to pass it along.

[DarthHater]

@flimzy @zendern I think adding this to a linter would be problematic, because OSS Index has rate limiting for one (and linting happens OFTEN), and you also don't change dependencies fairly often. It's likely a best spot for this would be in go build, etc..., so an occasional action