diff --git a/.github/workflows/on_pr_push.yml b/.github/workflows/on_pr_push.yml index 28d0b6e7..2217c5ee 100644 --- a/.github/workflows/on_pr_push.yml +++ b/.github/workflows/on_pr_push.yml @@ -203,6 +203,46 @@ jobs: if: always() run: docker compose down + # Its own job, so a formatting or vet failure is named in the checks list + # and arrives in seconds rather than after the fixture restore and the suite. + static: + name: Static checks + runs-on: ubuntu-22.04 + steps: + - name: Check out code + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version-file: 'go.mod' + check-latest: true + + # vendor/ is committed and is not `gofmt -s` clean, so `gofmt -s -l .` + # would report hundreds of files nobody here owns. List the tracked Go + # files outside it instead -- a filter on gofmt's output would have to + # survive grep exiting 1 on the clean run under the runner's pipefail. + - name: Check formatting + run: | + unformatted=$(git ls-files -z '*.go' ':!:vendor/**' | xargs -0 gofmt -s -l) + if [ -n "$unformatted" ]; then + echo "::error::these files are not gofmt -s clean:" + echo "$unformatted" + exit 1 + fi + + # The default analyzers, nothing added: the tree was brought clean against + # exactly these (MAPCO-11498), so a finding here is new. vet skips vendor/ + # on its own; ./... never matches it. + # + # Twice, because vet only sees the files the build tags select. The second + # run sets every opt-out tag plus pprof, so the stub files that stand in + # for a compiled-out backend are vetted too, not only the default build. + - name: Vet + run: | + go vet -mod vendor ./... + go vet -mod vendor -tags 'noS3Cache noRedisCache noAzblobCache noGCSCache noGpkgProvider noPostgisProvider noPrometheusObserver pprof' ./... + govulncheck: name: Run govulncheck runs-on: ubuntu-22.04 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9fe704ce..5fdcd1ef 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -281,8 +281,10 @@ Optional features compile out behind `noS3Cache`, `noRedisCache`, `noAzblobCache ## Code conventions -* **`gofmt -s`.** If running it produces changes in parts of the tree you are not working on, send - those in a separate pull request. +* **`gofmt -s` and `go vet` are required**, and CI enforces both (see [Required checks](#required-checks)). + Never run `gofmt -s -w .` at the root: it rewrites `vendor/`. Format the paths you changed, and if + that produces changes in parts of the tree you are not working on, send those in a separate pull + request. * **Error variables** take the form `var ErrErrorName = errors.New("provider: canceled")` — the text all lowercase, with no punctuation at the end. * **Table-driven subtests keyed by name**, with a `fn := func(tc tcase) func(*testing.T)` closure. @@ -324,13 +326,19 @@ CGO_ENABLED=0 go build -mod vendor ./... CGO_ENABLED=0 go test -run '^$' -mod vendor ./... # links every test binary, runs none ``` -Two more gates worth running before you push: +### Required checks + +CI fails a pull request on any of these, so run them before you push: ```bash -gofmt -s -l . | grep -v '^vendor/' # vendor/ is never -s clean; nothing else may appear +git ls-files -z '*.go' ':!:vendor/**' | xargs -0 gofmt -s -l # vendor/ is never -s clean; must print nothing +go vet -mod vendor ./... # the default analyzers; the tree is clean against them govulncheck ./... ``` +The tree is clean against `gofmt -s` and `go vet` today, so a finding from either is one your change +introduced. Fix it rather than working around the check. + ### Opt-in suites Provider- and backend-specific tests **skip unless you opt in** (`internal/ttools.ShouldSkip`), so a diff --git a/cmd/internal/register/maps_test.go b/cmd/internal/register/maps_test.go index f8b71ff1..ae81ceff 100644 --- a/cmd/internal/register/maps_test.go +++ b/cmd/internal/register/maps_test.go @@ -14,7 +14,6 @@ import ( func TestMaps(t *testing.T) { type tcase struct { - atlas atlas.Atlas maps []provider.Map providers []dict.Dict expectedErr error @@ -36,7 +35,10 @@ func TestMaps(t *testing.T) { return } - err = register.Maps(&tc.atlas, tc.maps, providers) + // A fresh atlas per case, not one carried in tcase: an Atlas holds a + // mutex, and a tcase is copied by value into fn and by range. + var a atlas.Atlas + err = register.Maps(&a, tc.maps, providers) if !errors.Is(err, tc.expectedErr) { t.Errorf("invalid error, expected %v got %v", tc.expectedErr, err) } diff --git a/cmd/shigola/cmd/tile_name_format_test.go b/cmd/shigola/cmd/tile_name_format_test.go index df17693c..2c415c8c 100644 --- a/cmd/shigola/cmd/tile_name_format_test.go +++ b/cmd/shigola/cmd/tile_name_format_test.go @@ -160,12 +160,13 @@ func TestFormatParse(t *testing.T) { for k, tc := range testcases { z, x, y, err := tc.format.Parse(tc.input) - if errOk(tc.err, err) { - continue - } else { + if !errOk(tc.err, err) { t.Errorf("[%v] unexpected err, expected %v got %v", k, tc.err, err) continue } + if tc.err != nil { + continue + } if z != tc.z || x != tc.x || y != tc.y { t.Errorf("[%v] expected output (z:%v, x:%v, y:%v) got (z:%v, x:%v, y:%v)", k, tc.z, tc.x, tc.y, z, x, y) diff --git a/internal/cmd/main.go b/internal/cmd/main.go index a30e286f..f2478d1d 100644 --- a/internal/cmd/main.go +++ b/internal/cmd/main.go @@ -157,7 +157,9 @@ func Signal() os.Signal { // can be passed in as well, if no list is passed os.Interrupt, and syscall.SIGTERM is // assumed. func NewContext(signals ...os.Signal) *contextType { - ch := make(chan os.Signal) + // Buffered, as signal.Notify requires: it never blocks on a send, so a signal + // arriving before signalHandler is receiving would otherwise be dropped. + ch := make(chan os.Signal, 1) if len(signals) == 0 { signal.Notify(ch, os.Interrupt, syscall.SIGTERM) } else { diff --git a/internal/env/dict.go b/internal/env/dict.go index ec98bef9..6454886d 100644 --- a/internal/env/dict.go +++ b/internal/env/dict.go @@ -78,7 +78,7 @@ func (d Dict) StringSlice(key string) (v []string, err error) { var iv []interface{} if iv, ok = val.([]interface{}); !ok { // Could not convert to the generic type, so we don't have the correct thing. - return v, dict.ErrKeyType{key, val, reflect.TypeOf(iv)} + return v, dict.ErrKeyType{Key: key, Value: val, T: reflect.TypeOf(iv)} } v = make([]string, len(iv)) @@ -156,7 +156,7 @@ func (d Dict) BoolSlice(key string) (v []bool, err error) { var iv []interface{} if iv, ok = val.([]interface{}); !ok { // Could not convert to the generic type, so we don't have the correct thing. - return v, dict.ErrKeyType{key, val, reflect.TypeOf(iv)} + return v, dict.ErrKeyType{Key: key, Value: val, T: reflect.TypeOf(iv)} } v = make([]bool, len(iv)) @@ -232,7 +232,7 @@ func (d Dict) IntSlice(key string) (v []int, err error) { var iv []interface{} if iv, ok = val.([]interface{}); !ok { // Could not convert to the generic type, so we don't have the correct thing. - return v, dict.ErrKeyType{key, val, reflect.TypeOf(iv)} + return v, dict.ErrKeyType{Key: key, Value: val, T: reflect.TypeOf(iv)} } v = make([]int, len(iv)) for k := range iv { @@ -436,7 +436,7 @@ func (d Dict) MapSlice(key string) (r []dict.Dicter, err error) { arr, ok := v.([]map[string]interface{}) if !ok { - return r, dict.ErrKeyType{key, v, reflect.TypeOf(arr)} + return r, dict.ErrKeyType{Key: key, Value: v, T: reflect.TypeOf(arr)} } r = make([]dict.Dicter, len(arr)) diff --git a/observability/prometheus/prometheus.go b/observability/prometheus/prometheus.go index b5899d90..78fd155b 100644 --- a/observability/prometheus/prometheus.go +++ b/observability/prometheus/prometheus.go @@ -133,8 +133,8 @@ func New(config dict.Dicter) (observability.Interface, error) { func (*observer) Name() string { return Name } -func (observer) Handler(string) http.Handler { return promhttp.Handler() } -func (obs *observer) Init() { obs.initCall.Do(obs.init) } +func (*observer) Handler(string) http.Handler { return promhttp.Handler() } +func (obs *observer) Init() { obs.initCall.Do(obs.init) } func (obs *observer) init() { obs.PublishBuildInfo() if obs == nil || obs.pushURL == "" {