Add linters to Makefile.common

[SuperQ]

Proposal

Add golint and gometalinter as targets in Makefile.common.

[SuperQ]

/cc @mjtrangoni

[brian-brazil]

Do we know what level of false positives we can expect?

[SuperQ]

Good question, Maybe @mjtrangoni has some more experience with that.

[mjtrangoni]

@brian-brazil you are right, there are going to be many false positives.
IMHO, the best and simplest way I found was using gometalinter plus a JSON configuration file, like this. This configuration file should be also project-specific.
Another interesting thing I found was that gometalinter introduces some specific flags for some linters. I mean, it is not only running the linter itself.
OTOH, I think this will be adding some great value to the projects.

[juliusv]

Yeah, careful with linting, at least https://github.com/golang/lint's README.md says "In short, this tool is not, and will never be, trustworthy enough for its suggestions to be enforced automatically, for example as part of a build process.", and we've known that for a while :)

We can still have targets for it of course, just not have them be part of CI checks.

[knweiss]

What I like about golint is that it e.g. ensures that exported functions and variables use CamelCase and have docstrings (i.e. it encourages Best Practices and idiomatic Go coding style). Issues like these are trivial to detect and fix during development but hard to fix after the new API is part of an official release (without breaking compatibility).

Calling golint in a release manager's Makefile target would be enough to catch these issues before release (but who's going to fix them?). Integrating golint in CI has the big advantage that each contributor would have to fix the issues before they even enter the repo. This keeps the commit history shorter and distributes the workload from the release manager to each individual contributor.

FWIW: At the moment prometheus master has 167 golint issues (132 of them in web/ui/bindata.go) . I wouldn't call them "false positives" but rather "opinionated" (and easy to fix).

$ golint ./... | grep -v ^vendor | wc -l
167

Some examples:

discovery/kubernetes/ingress.go:68:1: receiver name s should be consistent with previous receiver name e for Ingress
discovery/openstack/openstack.go:65:1: comment on exported type Role should be of the form "Role ..." (with optional leading article)
promql/lex.go:124:6: exported type ItemType should have comment or be unexported
web/api/v2/api.go:14:1: don't use an underscore in package name
web/ui/bindata.go:123:6: don't use underscores in Go names; func webUiTemplates_baseHtml should be webUITemplatesBaseHTML