Run golangci-lint as part of pull request workflow #604

louketo / louketo-proxy

A OpenID / Proxy service

Apache License 2.0

950 stars 343 forks source link

Run golangci-lint as part of pull request workflow #604 #607

Closed abstractj closed 4 years ago

abstractj commented 4 years ago

Changes

Merge build and linter jobs in a single file
Runs tests for go 1.13 and 1.14
Introduces code coverage with coveralls
Fixes some issues reported by golangci-lint and relax/disable some rules until the codebase is refactored

Running example:

https://github.com/abstractj/louketo-proxy/runs/676376256

Testing this PR

Download the build.yml file from this PR and add it to .github/workflows into the fork of Louketo
Push the changes to master at your fork
Submit a pull request to your own fork and GH action should be triggered

Relates to #615 Resolves #604

abstractj commented 4 years ago

Is there an easy way we can test these workflows before merging?

Yes, you can look at the fork here https://github.com/abstractj/louketo-proxy/actions. As you may notice, there are several alerts there for golangci-lint. Which ones do you think we should disable?

IMO the alerts for whitespace make sense, but not sure about the complexity warnings. Because sometimes we only updated the code to please the Linter. But if you consider this something important, we can keep it.

JoelSpeed commented 4 years ago

In general I think the complexity ones are good, but you can get ones that are a bit annoying, such as the ones highlighted here. I believe we can add an exception to .golangci.yaml if we want to keep it turned on. I have no strong opinion either way on this one

abstractj commented 4 years ago

@JoelSpeed np, let's keep it. Found a typo on my YAML file, but it's fixed now. If there's anything else that you would like to change, please let me know.

JoelSpeed commented 4 years ago

Do you think we should try and fixup the linting issues before merging this? Probably as a separate PR?

abstractj commented 4 years ago

@JoelSpeed of course, we can do that, there's no need to rush about merging this.

abstractj commented 4 years ago

@stianst could you please review?