Add custom go lint checker

[jpeach]

Please describe the problem you have

We have a few informal rules around style and interfaces

• CLI help should be capitalized
• log messages should not be capitalized
• import aliases should be have a consistent pattern

We can add a Go lint tool to check these conventions, removing the burden from maintainers and contributors.

[Glyphack]

I think there is a feature in golangci-lint that you can add custom plugins to it for your own needs. also by creating a plugin every repository can add it on top of golangci-lint to run.

[jpeach]

I think there is a feature in golangci-lint that you can add custom plugins to it for your own needs. also by creating a plugin every repository can add it on top of golangci-lint to run.

Yep, that would be my preference. Unfortunately it's a binary plugin interface, so distribution is a problem.

[Glyphack]

Yep, that would be my preference. Unfortunately it's a binary plugin interface, so distribution is a problem.

Yes it's binary plugin but it's possible to download binary in Makefile when running lint command. I take a look at this to see if I can help.

[Glyphack]

There are several examples of logs messages first letter capitalized. Are you sure this feature is needed?

[jpeach]

https://github.com/jpeach/go-message-fmt

[Glyphack]

Do you have an idea about import alias namimg pattern?

[jpeach]

Do you have an idea about import alias namimg pattern?

The alias naming guidelines are here. It might be hard to programmatically get it right because the rule is a bit subjective, but I expect it to reasonably easy to ensure that a set of specific well-known imports are always consistent.

[Glyphack]

I think this rule is easy to implement and also effective:
consider import path: github.com/projectcontour/x/y/z/v*

1. the alias name should be subset of x[optional]_y[optional]_z[optional]_v* where optional means it can be present or not. We can omit the last _ because It's already used like this

contour/apis/projectcontour/v1/kubeconditions.go

Line 25 in cc3076a

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

contour/cmd/contour/certgen.go

Line 26 in bb14b25

corev1 "k8s.io/api/core/v1"

2. one of x or y or z must be present in alias name. however this rule is not satisfied right now, for example:
   

   contour/cmd/contour/ingressstatus_test.go

   Line 20 in df5feec

   v1 "k8s.io/api/core/v1"

3. If version exists in path, must be specified.

4. words like apis should be api in import alias

[jpeach]

The rule is newer than the code, so it's not (yet) followed consistently. A linter would be really useful to help us make the transition (especially if it emits a fixit suggestion) :)

In your examples, I think it would be fine to end up with meta_v1 and core_v1 in all cases. It's less conventional, but more consistent.

[Glyphack]

Can I start working on implementation for import alias?

[jpeach]

Can I start working on implementation for import alias?

Yes, please do!

[jpeach]

On Oct 23, 2020, at 7:43 PM, Glyphack <notifications@github.com> wrote:  I was scanning the repo and found this is this a lint error we should catch? import envoy_api_v2_auth "github.com/envoyproxy/go-control-plane/envoy/api/v2/auth" error is raised because envoy does not match envoyproxy we can extend word match to a regex like *envoy*. I yeh this is why I suggested hard coding well known patterns. The rule isn’t described with enough precision to apply generally. We could end up with a config file entry that has the patterns, or just fixing things to confirm is ok by me.

[Glyphack]

@jpeach I finished the implementation of the linter. here's the PR, Can you take a look at it? also I attach the result of running this linter on the repo, seemed fine to me but double check is good.
res.txt
After this we can move on to the fixit feature.

[jpeach]

/home/glyphack/programming/contour/internal/certgen/certgen.go:25:2: used words in alias most be present in path path: k8s/io/apimachinery/pkg/api/errors alias: k8serrors

Unclear to me what the rule means for this import, maybe apimachinery_errors?

/home/glyphack/.cache/go-build/a3/a393284caaf6ccc1fa84f4413907252cfce43c3a6a389abdda771c0789515a66-d:13:2: used words in alias most be present in path path: github/com/projectcontour/contour/internal/contour alias: _test

Hmm, what is this telling me? Maybe an artifact of the testing mechanism?

/home/glyphack/programming/contour/internal/featuretests/envoy.go:29:2: version not specified in alias. path: github/com/envoyproxy/gocontrolplane/envoy/config/filter/network/tcp/proxy/v2 alias: envoy_config_v2_tcpproxy version v2

👍

Overall, it looks like the linter is doing a pretty reasonable job :)

[Glyphack]

Unclear to me what the rule means for this import, maybe apimachinery_errors?

Yes that's meant to be correct, maybe error message needs more clarification.

Hmm, what is this telling me? Maybe an artifact of the testing mechanism?

I suspect this is only happening on my machine could not find anything about it.

[Glyphack]

I implemented fixit option for linter(Glyphack/go-import-alias#2). and used it on contour main branch to see how it works(#3108). As I can see It's doing a reasonable job, Do you have any comments on it?

[Glyphack]

log messages should not be capitalized

a quick question @jpeach. what should happen to log messages like this?

contour/cmd/contour/serve.go

Lines 218 to 219 in 1b68b6d

	log.Warnf("error listing HTTPProxies: %v", err)
	} else {

you meant only the first letter should not be capitalized or the whole messge?

[Glyphack]

If we make a decision on how to integrate linter with project I can make progress on (#3108).
I guess there are two options:

1. using golangci-lint: the downside is that we have to use binary plugins.
2. using go-vet: downside is we have two source of linters(golangci-lint and go vet custom linters).

@jpeach @stevesloka

[jpeach]

@Glyphack WDYT about making a new repository for linters under projectcontour? Once we do that, we could look at the binary plugins, or we may be able to add it to golangci-lint directly?

[Glyphack]

@jpeach It's a good idea keeping list of all linters we use somewhere, Are you thinking about a single repo with all linters in it and a multicheker or separate ones? I'm OK with both.

Once we do that, we could look at the binary plugins, or we may be able to add it to golangci-lint directly?

I make an issue in golangci-lint about our linters, If they accept it to be part of golangci-lint it would be great, If not we can use binary plugins.

[jpeach]

@jpeach It's a good idea keeping list of all linters we use somewhere, Are you thinking about a single repo with all linters in it and a multicheker or separate ones? I'm OK with both.

yeh I was thinking of a single repo

[Glyphack]

I created this issue in golangci-lint repo 11 days ago and did not get any response from them. shall we create the linters repository and proceed with binary plugins?

[Glyphack]

I was trying to test linter using a generated plugin from github.com/contour/lint and got an error about cannot opening the plugin. searched and found this issue on golangci-lint:
golangci/golangci-lint#1276
seems like we can't use plugins with release versions of golangci-lint. any idea of how to proceed? we have two options here:

• using go vet
• building golangci-lint binary with CGO_ENABLED=1 and then run it.

[jpeach]

If we release this as a standalone binary, we can run in it in a separate Contour CI job, WDTY?

[sunjayBhatia]

closing for now, I think we're happing with our existing linting