Add gometalinter to build

[enocom]

No description provided.

[markmandel]

Just to add more details - gometalinter is in the build image for local usage, but the fun/tricky part will be ignoring the autogenerated code. The good thing is, all the autogenerated code does have an appropriate headers, so it should be straight forward to exclude them.

[EricFortin]

@enocom Have you started working on this? If not I’d like to help.

[enocom]

@EricFortin Go for it. I still haven't picked it up. I'll take my name off it.

[EricFortin]

@markmandel @enocom I have started looking into this and would like your input on how to implement this.

As it is, the master version of gometalinter is installed in the build image. The build could break anytime even if Agones' code is totally fine. We also run all default linters. When gometalinter adds a new one, it could also break the build for reasons unrelated to the current change being built.

In the doc, they recommend using a tagged version and specifying the linters you want to run. This is just a way to shield ourselves a little bit. WDYT?

I also saw that some linter default to warning for problem they find which means they aren't outputted and the build wouldn't fail. The most important one I think is the vetshadow one(equivalent to running go tool vet -shadow). I think we should put them as error since it really can change code execution. Do you agree?

[markmandel]

I'm not seeing any issues here - if nothing else, also happy for a build to simply fail if any linter fails.

gometalinter will let you put in a comment to ignore a specific line if you feel it's a false positive - so we can work around anything we specifically disagree with (unless it's generated code, as we talk about above, and we can --exclude that, I believe).

@enocom any extra thoughts on your end?

[enocom]

Making the build fail on vetshadow is a good idea. I'm in favor of getting it running and tuning it accordingly.

[markmandel]

I already have some // nolint declarations around the codebase as I tend to use gometalinter as I develop.

[EricFortin]

Hi again, I worked on this a little more and would like your opinion on some topics.

As an aside, I discovered that the vetshadow linter does indeed generate warnings and those are treated as errors(good) but I have been hit by the exact problem I was describing earlier where I was using an old master branch(my build image was quite old) that contained a bug that prevented it from reporting. I updated my build image and everything is fine now.

As for skipping the generated file, there is currently no way to skip a file based on its content. There is an issue opened on it but not much work done though.

gometalinter allows skipping folder or individual files and I made it work like this for my first iteration. Since @markmandel mentioned skipping on presence of go standard generated comment I am wondering if I should implement this. As stated, finding those files is easy but then I either need to generate a json config file or compose an enormous command line for gometalinter. What is your opinion on this?

[EricFortin]

Here is an example of my config file at the moment:

{
  "EnableAll": true,
  "skip": [
    "vendor",
    "client"
  ],
  "Exclude": [
    "pkg/apis/stable/v1alpha1/zz_generated.deepcopy.go"
  ],
  "deadline": "300s"
}

If I generated it, all filenames would be in the exclude list.

[enocom]

The config file seems reasonable, although I wonder if adding a // no lint directive as detailed here might produce a better result.

[EricFortin]

You would add it after we generate the file?

[enocom]

Yeah, as a minor and one time annoyance. The risk is subsequent file generations delete the line. But by all means, since you're seeing how painful each option is, let's go with what seems best to you.

[markmandel]

We add licence headers to all generated files - but they all use different methods. Making sure it hits just before the package declaration could be.... interesting 😄

Doing a grep for "autogenerated" and excluding those files is probably the best bet.

[EricFortin]

Good I’ll do the grep then. Thank you!