feat(ci): adding golangci-lint #35
Conversation
Signed-off-by: hjoshi123 <[email protected]>
|
Hi @hjoshi123 thank you for the change! Was there a particular reason you chose this set of checks? |
.golangci.yaml
Outdated
| gci: | ||
| sections: | ||
| - standard | ||
| - defaul |
.golangci.yaml
Outdated
| - exhaustive | ||
| - exptostd | ||
| - forbidigo | ||
| - ginkgolinter |
There was a problem hiding this comment.
I'm not sure we need this. We don't have any ginkgo tests in this repo.
scripts/install-tools.sh
Outdated
| ci_install=true | ||
| fi | ||
| if [[ "$ci_install" == true ]]; then | ||
| curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/cc3567e3127d8530afb69be1b7bd20ba9ebcc7c1/install.sh \ |
There was a problem hiding this comment.
In the other container project repos, we tend to install things under a .local directory for make to use (example https://github.com/apple/container/blob/9eb2f860fc35a933e7894bc5313e3b79cd001c52/Makefile#L162).
scripts/install-tools.sh
Outdated
| ci_install=true | ||
| fi | ||
| if [[ "$ci_install" == true ]]; then | ||
| curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/cc3567e3127d8530afb69be1b7bd20ba9ebcc7c1/install.sh \ |
There was a problem hiding this comment.
I think we could do something like this for the address
https://raw.githubusercontent.com/golangci/golangci-lint/${GOLANGCI_LINTER_VERSION}/install.sh
There was a problem hiding this comment.
Yup that makes sense.. changing it
Some of them were mentioned by default in the golangci lint docs and then I referred some of the k8s repos for others. Will make changes to remove some of them which are irrelevant as you mentioned. |
|
@hjoshi123 Were you able to make the changes here? |
|
@katiewasnothere sorry was busy with work.. will push the changes by this weekend. |
Signed-off-by: hjoshi123 <[email protected]>
|
|
||
| .PHONY: go-lint | ||
| go-lint: golangci-lint-install | ||
| $(GOLANGCI_LINT) config verify |
There was a problem hiding this comment.
I don't think this will work when the script has installed golangci-lint under .local/bin. I think following something similar to what we do for installing protoc would be good. What do you think?
|
|
||
| install_golangci() { | ||
| ci_install=false | ||
| if [[ -x $(which golangci-lint) ]]; then |
There was a problem hiding this comment.
I think which will not find the binary if it's installed under .local/bin
| if [[ "${version}" == "${GOLANGCI_LINTER_VERSION#v}" ]]; then | ||
| ci_install=false | ||
| else | ||
| ci_install=true |
There was a problem hiding this comment.
If we do something like how we install protoc, we get the check for whether to install golangci-lint for free since make will check if the file path exists, as it does for protoc here. Does that make sense?
| - name: Check protobufs | ||
| run: | | ||
| - name: Verify linter configuration and Lint go code | ||
| uses: golangci/golangci-lint-action@v8 |
There was a problem hiding this comment.
I think we should instead call the makefile command to install golangci-lint and then run it. That way the behavior is the same between local dev and the CI run.
There was a problem hiding this comment.
Also the CI right now won't run for this PR right now since golangci/golangci-lint-action@v8 is not in our allowlist for actions that can be run
|
Closing for now since there haven't been updates on this PR in the last month. Please feel free to reopen. |
2 participants
Fixes #27.
Added the following linters: