catch more bugs (e.g. unhandled errors) with ci golang linter

[kdorosh]

Gloo Edge Version

1.12.x (beta)

Kubernetes Version

No response

Describe the bug

add https://github.com/golangci/golangci-lint to ci

Steps to reproduce the bug

unknown bugs likely in code due to unhandled errors

Expected Behavior

ci should fail when we have obvious code errors

Additional Context

No response

[sam-heilbron]

https://pkg.go.dev/golang.org/x/exp/cmd/gorelease is another tool we could explore

[sam-heilbron]

Additional idea for a ginkgo validation courtesy of @ben-taussig-solo : https://github.com/solo-io/gloo/pull/9005#issuecomment-1864697999

[jbohanon]

Also (for posterity) https://github.com/solo-io/gloo/pull/9005#pullrequestreview-1789516162

[sam-heilbron]

There is an internal slack discussion around possible linters to add

Our general strategy is as follows (ordered):

In Gloo Open Source:

• [x] Support a basic ginkgo linter, with tests disabled
  • https://github.com/solo-io/gloo/pull/9005
• [x] Iteratively fix the tests
  • https://github.com/solo-io/gloo/pull/9007
• [x] Enable linting on tests in CI, this should be demonstrated to fail if a linting rule is broken
• [ ] Identify existing anti-patterns in tests (https://github.com/solo-io/gloo/issues/6686#issuecomment-1864706767), fix them, write linter rules to prevent them in the future
• [ ] Add additional linters progressively, and cleanup any code that can be removed as a result

In Gloo Enterprise:

• [ ] Demonstrate that open source is relatively stable, and up to date
• [ ] Copy the pattern used in open source

[jbohanon]

Linters that have piqued my interest and should be fairly quick wins:

• bodyclose https://github.com/solo-io/gloo/pull/9039
• exportloopref https://github.com/solo-io/gloo/pull/9043
• goimports https://github.com/solo-io/gloo/pull/9044
• nakedret https://github.com/solo-io/gloo/pull/9046
• predeclared https://github.com/solo-io/gloo/pull/9047
• unconvert https://github.com/solo-io/gloo/pull/9049
• unparam https://github.com/solo-io/gloo/pull/9051
• usestdlibvars https://github.com/solo-io/gloo/pull/9052

Linters that seemed like quick wins but are not (and why):

• nilerr
  • this linter checks that an error which was checked to be nonnil is returned instead of nil
  • we have several places where we comment why we're ignoring the error and returning nil anyway
  • a more robust pattern would be to return named errors and handle these cases in the caller explicitly
• nilnil
  • this linter checks that we are not returning an invalid value (nil) along with a nil error
  • this is mostly the same as above, and wins here would benefit nilerr as well
• errname https://github.com/solo-io/gloo/pull/9042
  • this ends up requiring that we change the names of exported vars and types which external users may depend upon
• musttag https://github.com/solo-io/gloo/pull/9045
  • the one failure of this linter in the gloo repo ends up being a non-trivial fix which requires investigation/discussion to proceed

[github-actions[bot]]

This issue has been marked as stale because of no activity in the last 180 days. It will be closed in the next 180 days unless it is tagged "no stalebot" or other activity occurs.