Add basic CI checks #2
Conversation
simonswine
commented
Jan 13, 2022
simonswine
commented
- golangci-lint
- misspell
- go test
simonswine
force-pushed
the
20220113_test-lint
branch
2 times, most recently
from
72f17ed
to
51eb738
Compare
simonswine
force-pushed
the
20220113_test-lint
branch
from
d33f858
to
4fecf48
Compare
| go run github.com/client9/misspell/cmd/misspell@v0.3.4 -error README.md LICENSE | ||
|
|
||
| # Configured via .golangci.yml. | ||
| go run github.com/golangci/golangci-lint/cmd/golangci-lint@v1.43.0 run |
There was a problem hiding this comment.
Is it a good idea to run golangci-lint from source? They specifically say to install a binary:
Note: such go get installation aren't guaranteed to work. We recommend using binary installation.
I think Grafana uses bingo to pin the golangci-lint version.
There was a problem hiding this comment.
Not too sure what we should aim for. The nice thing with go run is that you don't need to trust/validate binaries and they work on any supported os/architecture. I would rather have that benefit.
There was a problem hiding this comment.
@simonswine WDYT think of using bingo or Docker?
There was a problem hiding this comment.
Having thought a bit more, golangci-lint recommends using their own GitHub Action for CI, and locally Docker might be the best option. I'm frankly unsure if bingo installs from source, which would return us to the original problem.
The most worrying issue with installing golangci-lint from source that I can see would be:
go.mod replacement directive doesn't apply. It means a user will be using patched version of golangci-lint if we use such replacements.
There was a problem hiding this comment.
I think it is all theoretical problem as there are no replaces in the version of golangci-lint: https://github.com/golangci/golangci-lint/blob/v1.43.0/go.mod
I still think the go run is the most slim and still correct approach.
With a github action, how could I make sure the right version is used locally when I run make lint?
There was a problem hiding this comment.
Their point is they might add replaces in a future version though, and we probably won't think to check when upgrading.
With a github action, how could I make sure the right version is used locally when I run make lint?
Use their GitHub Action in CI, and use the recommended Docker command in Makefile.
There was a problem hiding this comment.
I am still very skeptical about the docker approach. It will have to download the image once and then potentially all the go modules everytime running the container. This takes quite a while:
# First run (docker image pull)
$ time docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint:v1.43.0 golangci-lint run -v
0.10s user 0.03s system 0% cpu 1:14.54 total
# Second run
0.04s user 0.01s system 0% cpu 21.918 total
Compared to go run:
# First run (go build cache empty)
$ time go run github.com/golangci/golangci-lint/cmd/golangci-lint@v1.43.0 run
go run github.com/golangci/golangci-lint/cmd/golangci-lint@v1.43.0 run 90.59s user 7.87s system 591% cpu 16.634 total
# Second run
$ time go run github.com/golangci/golangci-lint/cmd/golangci-lint@v1.43.0 run
go run github.com/golangci/golangci-lint/cmd/golangci-lint@v1.43.0 run 4.66s user 1.37s system 246% cpu 2.443 total
I think both of the runs with the container take significantly longer (and require the modules to be fetched on every run of golangci-lint). Also by using docker and the github action I need to have the version used specified twice (once in the workflow file and once the in the Makefile).
I think overall this is a worse approach than using go run and have potential problem when using an upgrade that replaced dependencies.
There was a problem hiding this comment.
@simonswine personally I prefer correctness/robustness (wrt. potential breakage when upgrading golangci-lint) over speed, especially when it comes to CI. I understand that this comes down to the opinion of those involved though. What do e.g. @pracucci and @pstibrany think?
There was a problem hiding this comment.
My personal preference is to build only our software, and leverage on existing builds when possible, mostly because it seems like waste of resources. But it's just a preference, not a hard rule.
There was a problem hiding this comment.
I personally don't care much. I wouldn't block on this. It's something we can revisit anytime.
| go run github.com/client9/misspell/cmd/misspell@v0.3.4 -error README.md LICENSE | ||
|
|
||
| # Configured via .golangci.yml. | ||
| go run github.com/golangci/golangci-lint/cmd/golangci-lint@v1.43.0 run |
There was a problem hiding this comment.
I personally don't care much. I wouldn't block on this. It's something we can revisit anytime.
