CI: bump github actions, simplify things #1476
Conversation
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Prealloc linter is disabled by default, so it does not make sense to: - have it disabled explicitly; - have any configuration for it. Should not result in any changes to CI runs. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These two linters are not enabled, so it does not make sense to have any configuration for them. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The options configured (check-type-assertions and check-blank) default to false, so it does not make sense to configure them explicitly. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. We do not have any formatters configured, so remove formatters configuration. 2. We do not have any paths like those excluded in configuration, so remove the excluded paths. 3. Remove "exclusions / generated: lax" as we don't have any generated code, and the default of "generated: strict" is fine. 4. Remove "comments" and "common-false-positives" from "exclusions / presets" as they don't hide any warnings. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. bodyclose (https://github.com/timakin/bodyclose) validates whether *net/http.Response of HTTP request calls method Body.Close(), and we only use it in a single test (and linting test files are disabled in configuration). 2. protogetter (https://github.com/ghostiam/protogetter) -- we do not use protobuf. 3. rowserrcheck (https://github.com/jingyugao/rowserrcheck) and sqlclosecheck (https://github.com/ryanrolds/sqlclosecheck) -- we don't use SQL. 4. spancheck (https://github.com/jjti/go-spancheck) -- we don't use OpenTelemetry nor OpenCensus. 5. testifylint (https://github.com/Antonboom/testifylint) checks the code that uses github.com/stretchr/testify, which is only used in tests, and we have test file linting disabled. 6. zerologlint (https://github.com/ykadowak/zerologlint) -- we don't use zerolog. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
AFAIK travis-ci is not used for this repo for quite some time, so its scripts and config can be safely removed. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
I guess this can be split into 3 or 4 more digestible PRs (bumps, golangci-lint, removal) -- let me know if you want me to split it @thaJeztah |
| - rowserrcheck | ||
| - spancheck | ||
| - sqlclosecheck | ||
| - testifylint |
There was a problem hiding this comment.
Perhaps we should enable this one for tests though; I recall there was a PR that fixed issues reported by it, so would be good to have it run in CI to make sure we don't regress;
There was a problem hiding this comment.
Let's address this as a followup -- the focus of this one is to simplify things / remove useless configuration. If I add some linters for test files, this PR will probably grow much bigger (and harder to review).
I just checked, if we lint test files (and leave testifylint), we get 19 issues, 7 of those for testifylint. I can fix all those but not in this PR.
There was a problem hiding this comment.
OK I have three more commits that enable some linters for test files (including testifylint) and fix some of their warnings. Let me know if you want those commits here in this PR @thaJeztah
There was a problem hiding this comment.
Let's bring this PR in; ok or a follow-up 👍
| - asasalint | ||
| - asciicheck | ||
| - bidichk | ||
| - bodyclose |
There was a problem hiding this comment.
Should we enable linting on tests? I agree that in this codebase, bodyclose is less useful, then again, I've seen it catch some things that were overlooked, and the codebase is relatively small so not a huge concern to run more linters?
| @@ -1,15 +0,0 @@ | |||
| language: go | |||
There was a problem hiding this comment.
Ah! Good find; I thought I removed it, but that was AppVeyor (but had to add it back because I don't have access to settings to disable it 😓 #1465)
.github/workflows/ci.yaml
Outdated
| - name: Build | ||
| run: | | ||
| for target in linux/amd64 darwin/amd64 darwin/arm64 freebsd/amd64 js/wasm solaris/amd64 windows/amd64 windows/arm64; do | ||
| echo "Building for $target" | ||
| GOOS=${target%/*} GOARCH=${target#*/} go build ./... |
There was a problem hiding this comment.
Perhaps this one would be good to move to a separate PR; effectively this reverts #1208
Admitted, I wasn't a fan of adding that as dependency (so I moved it to a separate module in a follow-up), but there's pros and cons to shell script.
(That said, perhaps if we would stick with mage, we should bump the go version and make sure we add more cross targets?)
There was a problem hiding this comment.
Ah, right, yes it did.
I wonder if we should / could use Go itself to enumerate cross-compile targets; possibly with an exclusion list for those that can't work; I did a quick search, and it looks like go tool dist list can enumerate all options;
go tool dist list
aix/ppc64
android/386
android/amd64
android/arm
android/arm64
darwin/amd64
darwin/arm64
dragonfly/amd64
freebsd/386
freebsd/amd64
freebsd/arm
freebsd/arm64
freebsd/riscv64
illumos/amd64
ios/amd64
ios/arm64
js/wasm
linux/386
linux/amd64
linux/arm
linux/arm64
linux/loong64
linux/mips
linux/mips64
linux/mips64le
linux/mipsle
linux/ppc64
linux/ppc64le
linux/riscv64
linux/s390x
netbsd/386
netbsd/amd64
netbsd/arm
netbsd/arm64
openbsd/386
openbsd/amd64
openbsd/arm
openbsd/arm64
openbsd/ppc64
openbsd/riscv64
plan9/386
plan9/amd64
plan9/arm
solaris/amd64
wasip1/wasm
windows/386
windows/amd64
windows/arm64
|
Removed the last commit away from here; will work on it once this PR will be merged. |
2 participants
For details, see individual commits. High level overview:
.golangci.yml(no functional changes);removemagethingie.