[MM-49410] Added golangci-lint support

[fmartingr]

Summary

Added golangci-lint support

Ticket Link

https://mattermost.atlassian.net/browse/MM-49410

Release Note

none

[fmartingr]

PR fixes everything at this point except for shadowing. Enabling shadowing outputs a lot more errors around (most of them err in tests, but there are other outside the test suite). Yesterday I told you that we should discuss if disabling it on tests, but after a bit of thought I'd rather enable or disable it globally to avoid confusion. And I'd say it has more value enabled rather than disabled. What are your thoughts @mirshahriar @gabrieljackson ? Also if we could get @agnivade to chip in as well, since its sample configuration file has shadowing enabled, would love your opinion here.

[mirshahriar]

Do we need to skip internal/mocks in make lint?

[fmartingr]

Do we need to skip internal/mocks in make lint?

That's why I'd like to discuss. Missread.

yeah, since mocks code is autogenerated, there's no point in linting it, since we're not going to fix the lint errors each time we run make mocks.

[agnivade]

From my experience, shadowing is one of those issues which are a pain to fix, but sometimes a bug eventually falls through. So it's not very urgent, but I'd suggest to fix them. What we usually do in server is to rename err to err2 .

This was a real bug that we fixed caused due to shadowing: https://github.com/mattermost/enterprise/pull/594/files. It might look simple now but sometimes it's easy to miss.

[fmartingr]

[gabrieljackson]

I am okay with us setting the shadow check to disabled initially so we can get this running in CI/CD. We can then do some follow-up changes to correct shadowing issues and enable enforcement with shadow-checking.

[fmartingr]

I am okay with us setting the shadow check to disabled initially so we can get this running in CI/CD. We can then do some follow-up changes to correct shadowing issues and enable enforcement with shadow-checking.

I'm already taking care of all the shadowing issues, I wanted to have it done by today but other things came up. I will have it ready by monday.

[fmartingr]

[mirshahriar]

I agree with turning on the shadow check. But I'm not a fan of using err2, err3 when err would suffice. If we need to introduce a new error variable to remove shadowing, I am in favour of naming it after the relevant error.

Like

  _, err2 := exec.LookPath(extraTool)
  if err2 != nil {
	  return errors.Errorf("failed to find %s on the PATH", extraTool)
  }

Instead of using err2, we can use execErr

_, execErr := exec.LookPath(extraTool)
if execErr != nil {
	return errors.Errorf("failed to find %s on the PATH", extraTool)
}

Though both are the same.

[fmartingr]

I agree with turning on the shadow check. But I'm not a fan of using err2, err3 when err would suffice. If we need to introduce a new error variable to remove shadowing, I am in favour of naming it after the relevant error.

I agree with that, and I think I tried to use a similar naming outside of tests (so logic makes sense) but I may have missed some since most changes were mechanical after the first dozen or so. If you have spotted some tell me and I'll change it.

[gabrieljackson]

LGTM! There are a few err2 type names outside of tests that I don't love, but this is totally fine. We can just change them later if they bug us.

[gabrieljackson]

C'mon linter! It says right there it's unused. Why do you just leave it alone!? 😂

[mirshahriar]

LGTM. But I would love to see the changes in err2, err3 naming in future.