search

valkey-io / valkey-go

A fast Golang Valkey client that supports Client Side Caching and Auto Pipelining.
Apache License 2.0
157 stars 10 forks source link

GH action on PR and tests #6

Closed szuecs closed 6 months ago

szuecs commented 6 months ago

As far as I see the current GH actions are configured for only push action. It would be great to be able to see the test is passing and maybe add some linting to the PR, such that we can increase code quality.

For example we use vet and staticcheck https://github.com/zalando/skipper/blob/master/.github/workflows/pr.yaml Of course we have to disable some checks because we can not just break lib users for the sake of a linter https://github.com/zalando/skipper/blob/master/Makefile#L129-L138

For security checks I can recommend https://github.com/zalando/skipper/blob/master/Makefile#L150-L160 , gosec I can not recommend, because of an annoying false positive rate, but govulncheck, capslock and osv-scanner are really great.

rueian commented 6 months ago

Hi @szuecs,

I have re-enabled GH actions on pull requests https://github.com/valkey-io/valkey-go/commit/3eb1b2c35c6b3f33cfdb5ab755abb77c40a78a00.

I found that vet and staticcheck with default rules and excluding tests, by grep -v not -tests=false, are good:

go vet ./...
staticcheck  ./... | (grep -v "_test.go:" && exit 1 || exit 0)
image

Their results are actionable, with no false positives and no unnecessary alerts.

Would you be willing to add them to the current https://github.com/valkey-io/valkey-go/blob/main/dockertest.sh?

On the other hand, the results of govulncheck, capslock and osv-scanner are quite confusing to me. They seem to be most related to dependencies including Go itself. That makes me feel we don't need to add them to RP checks since we already have Dependabot configured.

szuecs commented 6 months ago

@rueian I disabled some checks and added comments on why. In the future we can fix some of them and others we will have to accept or add opt-out comments to API breaking change proposals that we do not want to do. One tiny code change to fix a linter finding is hopefully fine for this.