From 45506841e664665e8f0b1b59f416c91643913a3f Mon Sep 17 00:00:00 2001 From: Simon Pasquier Date: Fri, 3 May 2019 15:11:28 +0200 Subject: [PATCH] *: enable all default linters (#5504) Signed-off-by: Simon Pasquier --- .circleci/config.yml | 6 +++- .golangci.yml | 15 +++++--- Makefile.common | 3 +- cmd/promtool/http.go | 45 ------------------------ cmd/promtool/http_test.go | 56 ------------------------------ cmd/promtool/main.go | 3 ++ discovery/consul/consul.go | 24 ++++++------- discovery/kubernetes/endpoints.go | 14 ++++---- promql/ast.go | 1 + promql/engine_test.go | 1 + promql/parse_test.go | 1 + rules/manager.go | 4 +-- rules/manager_test.go | 2 +- scrape/target.go | 2 ++ scripts/errcheck_excludes.txt | 13 +++++++ storage/remote/wal_watcher_test.go | 14 ++++---- template/template_test.go | 20 +++++------ util/strutil/quote_test.go | 12 +++---- web/api/v1/api.go | 5 ++- web/api/v1/api_test.go | 2 +- web/federate.go | 2 +- 21 files changed, 87 insertions(+), 158 deletions(-) delete mode 100644 cmd/promtool/http.go delete mode 100644 cmd/promtool/http_test.go create mode 100644 scripts/errcheck_excludes.txt diff --git a/.circleci/config.yml b/.circleci/config.yml index ea8b1804e..486258289 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -15,7 +15,11 @@ jobs: steps: - checkout - run: make promu - - run: make check_license style unused lint build check_assets + - run: + command: make check_license style unused lint build check_assets + environment: + # Run garbage collection more aggresively to avoid getting OOMed during the lint phase. + GOGC: "20" - run: command: | curl -s -L https://github.com/protocolbuffers/protobuf/releases/download/v3.5.1/protoc-3.5.1-linux-x86_64.zip > /tmp/protoc.zip diff --git a/.golangci.yml b/.golangci.yml index fd975b2dd..1a05236e2 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,8 +1,13 @@ run: modules-download-mode: vendor + deadline: 5m -# Run only staticcheck for now. Additional linters will be enabled one-by-one. -linters: - enable: - - staticcheck - disable-all: true +issues: + exclude-rules: + - path: _test.go + linters: + - errcheck + +linters-settings: + errcheck: + exclude: scripts/errcheck_excludes.txt diff --git a/Makefile.common b/Makefile.common index 73052b3c0..4f18ea587 100644 --- a/Makefile.common +++ b/Makefile.common @@ -73,6 +73,7 @@ PROMU_VERSION ?= 0.3.0 PROMU_URL := https://github.com/prometheus/promu/releases/download/v$(PROMU_VERSION)/promu-$(PROMU_VERSION).$(GO_BUILD_PLATFORM).tar.gz GOLANGCI_LINT := +GOLANGCI_LINT_OPTS ?= GOLANGCI_LINT_VERSION ?= v1.16.0 # golangci-lint only supports linux, darwin and windows platforms on i386/amd64. # windows isn't included here because of the path separator being different. @@ -166,7 +167,7 @@ ifdef GO111MODULE # 'go list' needs to be executed before staticcheck to prepopulate the modules cache. # Otherwise staticcheck might fail randomly for some reason not yet explained. GO111MODULE=$(GO111MODULE) $(GO) list -e -compiled -test=true -export=false -deps=true -find=false -tags= -- ./... > /dev/null - GO111MODULE=$(GO111MODULE) $(GOLANGCI_LINT) run $(pkgs) + GO111MODULE=$(GO111MODULE) $(GOLANGCI_LINT) run $(GOLANGCI_LINT_OPTS) $(pkgs) else $(GOLANGCI_LINT) run $(pkgs) endif diff --git a/cmd/promtool/http.go b/cmd/promtool/http.go deleted file mode 100644 index e269a23ed..000000000 --- a/cmd/promtool/http.go +++ /dev/null @@ -1,45 +0,0 @@ -// Copyright 2015 The Prometheus Authors -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package main - -import ( - "time" - - "github.com/pkg/errors" - "github.com/prometheus/client_golang/api" -) - -const defaultTimeout = 2 * time.Minute - -type prometheusHTTPClient struct { - requestTimeout time.Duration - httpClient api.Client -} - -func newPrometheusHTTPClient(serverURL string) (*prometheusHTTPClient, error) { - hc, err := api.NewClient(api.Config{ - Address: serverURL, - }) - if err != nil { - return nil, errors.Wrapf(err, "error creating HTTP client") - } - return &prometheusHTTPClient{ - requestTimeout: defaultTimeout, - httpClient: hc, - }, nil -} - -func (c *prometheusHTTPClient) urlJoin(path string) string { - return c.httpClient.URL(path, nil).String() -} diff --git a/cmd/promtool/http_test.go b/cmd/promtool/http_test.go deleted file mode 100644 index b9783daaf..000000000 --- a/cmd/promtool/http_test.go +++ /dev/null @@ -1,56 +0,0 @@ -// Copyright 2015 The Prometheus Authors -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package main - -import "testing" - -func TestURLJoin(t *testing.T) { - - testCases := []struct { - inputHost string - inputPath string - expected string - }{ - {"http://host", "path", "http://host/path"}, - {"http://host", "path/", "http://host/path"}, - {"http://host", "/path", "http://host/path"}, - {"http://host", "/path/", "http://host/path"}, - - {"http://host/", "path", "http://host/path"}, - {"http://host/", "path/", "http://host/path"}, - {"http://host/", "/path", "http://host/path"}, - {"http://host/", "/path/", "http://host/path"}, - - {"https://host", "path", "https://host/path"}, - {"https://host", "path/", "https://host/path"}, - {"https://host", "/path", "https://host/path"}, - {"https://host", "/path/", "https://host/path"}, - - {"https://host/", "path", "https://host/path"}, - {"https://host/", "path/", "https://host/path"}, - {"https://host/", "/path", "https://host/path"}, - {"https://host/", "/path/", "https://host/path"}, - } - for i, c := range testCases { - client, err := newPrometheusHTTPClient(c.inputHost) - if err != nil { - panic(err) - } - actual := client.urlJoin(c.inputPath) - if actual != c.expected { - t.Errorf("Error on case %d: %v(actual) != %v(expected)", i, actual, c.expected) - } - t.Logf("Case %d: %v(actual) == %v(expected)", i, actual, c.expected) - } -} diff --git a/cmd/promtool/main.go b/cmd/promtool/main.go index 0759d46d3..51d5a6ca6 100644 --- a/cmd/promtool/main.go +++ b/cmd/promtool/main.go @@ -619,11 +619,14 @@ func (p *promqlPrinter) printLabelValues(val model.LabelValues) { type jsonPrinter struct{} func (j *jsonPrinter) printValue(v model.Value) { + //nolint:errcheck json.NewEncoder(os.Stdout).Encode(v) } func (j *jsonPrinter) printSeries(v []model.LabelSet) { + //nolint:errcheck json.NewEncoder(os.Stdout).Encode(v) } func (j *jsonPrinter) printLabelValues(v model.LabelValues) { + //nolint:errcheck json.NewEncoder(os.Stdout).Encode(v) } diff --git a/discovery/consul/consul.go b/discovery/consul/consul.go index 86e251a63..773e82236 100644 --- a/discovery/consul/consul.go +++ b/discovery/consul/consul.go @@ -341,7 +341,7 @@ func (d *Discovery) Run(ctx context.Context, ch chan<- []*targetgroup.Group) { // Watch the catalog for new services we would like to watch. This is called only // when we don't know yet the names of the services and need to ask Consul the // entire list of services. -func (d *Discovery) watchServices(ctx context.Context, ch chan<- []*targetgroup.Group, lastIndex *uint64, services map[string]func()) error { +func (d *Discovery) watchServices(ctx context.Context, ch chan<- []*targetgroup.Group, lastIndex *uint64, services map[string]func()) { catalog := d.client.Catalog() level.Debug(d.logger).Log("msg", "Watching services", "tags", d.watchedTags) @@ -360,11 +360,11 @@ func (d *Discovery) watchServices(ctx context.Context, ch chan<- []*targetgroup. level.Error(d.logger).Log("msg", "Error refreshing service list", "err", err) rpcFailuresCount.Inc() time.Sleep(retryInterval) - return err + return } // If the index equals the previous one, the watch timed out with no update. if meta.LastIndex == *lastIndex { - return nil + return } *lastIndex = meta.LastIndex @@ -396,12 +396,11 @@ func (d *Discovery) watchServices(ctx context.Context, ch chan<- []*targetgroup. // Send clearing target group. select { case <-ctx.Done(): - return ctx.Err() + return case ch <- []*targetgroup.Group{{Source: name}}: } } } - return nil } // consulService contains data belonging to the same service. @@ -441,14 +440,17 @@ func (d *Discovery) watchService(ctx context.Context, ch chan<- []*targetgroup.G return default: srv.watch(ctx, ch, catalog, &lastIndex) - <-ticker.C + select { + case <-ticker.C: + case <-ctx.Done(): + } } } }() } // Get updates for a service. -func (srv *consulService) watch(ctx context.Context, ch chan<- []*targetgroup.Group, catalog *consul.Catalog, lastIndex *uint64) error { +func (srv *consulService) watch(ctx context.Context, ch chan<- []*targetgroup.Group, catalog *consul.Catalog, lastIndex *uint64) { level.Debug(srv.logger).Log("msg", "Watching service", "service", srv.name, "tags", srv.tags) t0 := time.Now() @@ -465,7 +467,7 @@ func (srv *consulService) watch(ctx context.Context, ch chan<- []*targetgroup.Gr // Check the context before in order to exit early. select { case <-ctx.Done(): - return ctx.Err() + return default: // Continue. } @@ -474,11 +476,11 @@ func (srv *consulService) watch(ctx context.Context, ch chan<- []*targetgroup.Gr level.Error(srv.logger).Log("msg", "Error refreshing service", "service", srv.name, "tags", srv.tags, "err", err) rpcFailuresCount.Inc() time.Sleep(retryInterval) - return err + return } // If the index equals the previous one, the watch timed out with no update. if meta.LastIndex == *lastIndex { - return nil + return } *lastIndex = meta.LastIndex @@ -536,8 +538,6 @@ func (srv *consulService) watch(ctx context.Context, ch chan<- []*targetgroup.Gr select { case <-ctx.Done(): - return ctx.Err() case ch <- []*targetgroup.Group{&tgroup}: } - return nil } diff --git a/discovery/kubernetes/endpoints.go b/discovery/kubernetes/endpoints.go index d97e19204..ca7f31323 100644 --- a/discovery/kubernetes/endpoints.go +++ b/discovery/kubernetes/endpoints.go @@ -325,11 +325,12 @@ func (e *Endpoints) resolvePodRef(ref *apiv1.ObjectReference) *apiv1.Pod { p.Name = ref.Name obj, exists, err := e.podStore.Get(p) - if err != nil || !exists { - return nil - } if err != nil { level.Error(e.logger).Log("msg", "resolving pod ref failed", "err", err) + return nil + } + if !exists { + return nil } return obj.(*apiv1.Pod) } @@ -340,11 +341,12 @@ func (e *Endpoints) addServiceLabels(ns, name string, tg *targetgroup.Group) { svc.Name = name obj, exists, err := e.serviceStore.Get(svc) - if !exists || err != nil { - return - } if err != nil { level.Error(e.logger).Log("msg", "retrieving service failed", "err", err) + return + } + if !exists { + return } svc = obj.(*apiv1.Service) diff --git a/promql/ast.go b/promql/ast.go index e93cac2b6..3cc699aa3 100644 --- a/promql/ast.go +++ b/promql/ast.go @@ -323,5 +323,6 @@ func (f inspector) Visit(node Node, path []Node) (Visitor, error) { // f(node, path); node must not be nil. If f returns a nil error, Inspect invokes f // for all the non-nil children of node, recursively. func Inspect(node Node, f inspector) { + //nolint: errcheck Walk(inspector(f), node, nil) } diff --git a/promql/engine_test.go b/promql/engine_test.go index 68481ed85..5b64e8074 100644 --- a/promql/engine_test.go +++ b/promql/engine_test.go @@ -791,6 +791,7 @@ func TestRecoverEvaluatorRuntime(t *testing.T) { // Cause a runtime panic. var a []int + //nolint:govet a[123] = 1 if err.Error() != "unexpected error" { diff --git a/promql/parse_test.go b/promql/parse_test.go index 5e1562024..ec4711916 100644 --- a/promql/parse_test.go +++ b/promql/parse_test.go @@ -1789,6 +1789,7 @@ func TestRecoverParserRuntime(t *testing.T) { defer p.recover(&err) // Cause a runtime panic. var a []int + //nolint:govet a[123] = 1 } diff --git a/rules/manager.go b/rules/manager.go index 77130489f..1dc72cc87 100644 --- a/rules/manager.go +++ b/rules/manager.go @@ -355,8 +355,8 @@ func (g *Group) stop() { func (g *Group) hash() uint64 { l := labels.New( - labels.Label{"name", g.name}, - labels.Label{"file", g.file}, + labels.Label{Name: "name", Value: g.name}, + labels.Label{Name: "file", Value: g.file}, ) return l.Hash() } diff --git a/rules/manager_test.go b/rules/manager_test.go index 6298c0866..bf2bfca6c 100644 --- a/rules/manager_test.go +++ b/rules/manager_test.go @@ -552,7 +552,7 @@ func TestStaleness(t *testing.T) { metricSample[2].V = 42 // reflect.DeepEqual cannot handle NaN. want := map[string][]promql.Point{ - metric: {{0, 2}, {1000, 3}, {2000, 42}}, + metric: {{T: 0, V: 2}, {T: 1000, V: 3}, {T: 2000, V: 42}}, } testutil.Equals(t, want, samples) diff --git a/scrape/target.go b/scrape/target.go index f210c46ba..acf1eec3b 100644 --- a/scrape/target.go +++ b/scrape/target.go @@ -118,7 +118,9 @@ func (t *Target) setMetadataStore(s metricMetadataStore) { // hash returns an identifying hash for the target. func (t *Target) hash() uint64 { h := fnv.New64a() + //nolint: errcheck h.Write([]byte(fmt.Sprintf("%016d", t.labels.Hash()))) + //nolint: errcheck h.Write([]byte(t.URL().String())) return h.Sum64() diff --git a/scripts/errcheck_excludes.txt b/scripts/errcheck_excludes.txt new file mode 100644 index 000000000..e31da6f67 --- /dev/null +++ b/scripts/errcheck_excludes.txt @@ -0,0 +1,13 @@ +// Don't flag lines such as "io.Copy(ioutil.Discard, resp.Body)". +io.Copy +// The next two are used in HTTP handlers, any error is handled by the server itself. +io.WriteString +(net/http.ResponseWriter).Write +// No need to check for errors on server's shutdown. +(*net/http.Server).Shutdown + +// Never check for logger errors. +(github.com/go-kit/kit/log.Logger).Log + +// Never check for rollback errors as Rollback() is called when a previous error was detected. +(github.com/prometheus/prometheus/storage.Appender).Rollback diff --git a/storage/remote/wal_watcher_test.go b/storage/remote/wal_watcher_test.go index 31f27d380..2c584f6d7 100644 --- a/storage/remote/wal_watcher_test.go +++ b/storage/remote/wal_watcher_test.go @@ -112,7 +112,7 @@ func TestTailSamples(t *testing.T) { series := enc.Series([]tsdb.RefSeries{ tsdb.RefSeries{ Ref: uint64(ref), - Labels: labels.Labels{labels.Label{"__name__", fmt.Sprintf("metric_%d", i)}}, + Labels: labels.Labels{labels.Label{Name: "__name__", Value: fmt.Sprintf("metric_%d", i)}}, }, }, nil) testutil.Ok(t, w.Log(series)) @@ -182,7 +182,7 @@ func TestReadToEndNoCheckpoint(t *testing.T) { series := enc.Series([]tsdb.RefSeries{ tsdb.RefSeries{ Ref: uint64(i), - Labels: labels.Labels{labels.Label{"__name__", fmt.Sprintf("metric_%d", i)}}, + Labels: labels.Labels{labels.Label{Name: "__name__", Value: fmt.Sprintf("metric_%d", i)}}, }, }, nil) recs = append(recs, series) @@ -246,7 +246,7 @@ func TestReadToEndWithCheckpoint(t *testing.T) { series := enc.Series([]tsdb.RefSeries{ tsdb.RefSeries{ Ref: uint64(ref), - Labels: labels.Labels{labels.Label{"__name__", fmt.Sprintf("metric_%d", i)}}, + Labels: labels.Labels{labels.Label{Name: "__name__", Value: fmt.Sprintf("metric_%d", i)}}, }, }, nil) testutil.Ok(t, w.Log(series)) @@ -272,7 +272,7 @@ func TestReadToEndWithCheckpoint(t *testing.T) { series := enc.Series([]tsdb.RefSeries{ tsdb.RefSeries{ Ref: uint64(i), - Labels: labels.Labels{labels.Label{"__name__", fmt.Sprintf("metric_%d", i)}}, + Labels: labels.Labels{labels.Label{Name: "__name__", Value: fmt.Sprintf("metric_%d", i)}}, }, }, nil) testutil.Ok(t, w.Log(series)) @@ -328,7 +328,7 @@ func TestReadCheckpoint(t *testing.T) { series := enc.Series([]tsdb.RefSeries{ tsdb.RefSeries{ Ref: uint64(ref), - Labels: labels.Labels{labels.Label{"__name__", fmt.Sprintf("metric_%d", i)}}, + Labels: labels.Labels{labels.Label{Name: "__name__", Value: fmt.Sprintf("metric_%d", i)}}, }, }, nil) testutil.Ok(t, w.Log(series)) @@ -391,7 +391,7 @@ func TestReadCheckpointMultipleSegments(t *testing.T) { series := enc.Series([]tsdb.RefSeries{ tsdb.RefSeries{ Ref: uint64(ref), - Labels: labels.Labels{labels.Label{"__name__", fmt.Sprintf("metric_%d", j)}}, + Labels: labels.Labels{labels.Label{Name: "__name__", Value: fmt.Sprintf("metric_%d", j)}}, }, }, nil) testutil.Ok(t, w.Log(series)) @@ -458,7 +458,7 @@ func TestCheckpointSeriesReset(t *testing.T) { series := enc.Series([]tsdb.RefSeries{ tsdb.RefSeries{ Ref: uint64(ref), - Labels: labels.Labels{labels.Label{"__name__", fmt.Sprintf("metric_%d", i)}}, + Labels: labels.Labels{labels.Label{Name: "__name__", Value: fmt.Sprintf("metric_%d", i)}}, }, }, nil) testutil.Ok(t, w.Log(series)) diff --git a/template/template_test.go b/template/template_test.go index d7ce8c4a3..caf816151 100644 --- a/template/template_test.go +++ b/template/template_test.go @@ -25,18 +25,16 @@ import ( "github.com/prometheus/prometheus/util/testutil" ) -type testTemplatesScenario struct { - text string - output string - input interface{} - queryResult promql.Vector - shouldFail bool - html bool - errorMsg string -} - func TestTemplateExpansion(t *testing.T) { - scenarios := []testTemplatesScenario{ + scenarios := []struct { + text string + output string + input interface{} + queryResult promql.Vector + shouldFail bool + html bool + errorMsg string + }{ { // No template. text: "plain text", diff --git a/util/strutil/quote_test.go b/util/strutil/quote_test.go index 32aea0940..6fde4ca14 100644 --- a/util/strutil/quote_test.go +++ b/util/strutil/quote_test.go @@ -17,13 +17,11 @@ import ( "testing" ) -type quoteTest struct { +var quotetests = []struct { in string out string ascii string -} - -var quotetests = []quoteTest{ +}{ {"\a\b\f\r\n\t\v", `"\a\b\f\r\n\t\v"`, `"\a\b\f\r\n\t\v"`}, {"\\", `"\\"`, `"\\"`}, {"abc\xffdef", `"abc\xffdef"`, `"abc\xffdef"`}, @@ -32,12 +30,10 @@ var quotetests = []quoteTest{ {"\x04", `"\x04"`, `"\x04"`}, } -type unQuoteTest struct { +var unquotetests = []struct { in string out string -} - -var unquotetests = []unQuoteTest{ +}{ {`""`, ""}, {`"a"`, "a"}, {`"abc"`, "abc"}, diff --git a/web/api/v1/api.go b/web/api/v1/api.go index 08a1c4d0b..bfb4aec9f 100644 --- a/web/api/v1/api.go +++ b/web/api/v1/api.go @@ -838,7 +838,10 @@ func (api *API) serveFlags(r *http.Request) apiFuncResult { } func (api *API) remoteRead(w http.ResponseWriter, r *http.Request) { - api.remoteReadGate.Start(r.Context()) + if err := api.remoteReadGate.Start(r.Context()); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } remoteReadQueries.Inc() defer api.remoteReadGate.Done() diff --git a/web/api/v1/api_test.go b/web/api/v1/api_test.go index b2b8939de..11d2802b1 100644 --- a/web/api/v1/api_test.go +++ b/web/api/v1/api_test.go @@ -884,7 +884,7 @@ func assertAPIError(t *testing.T, got *apiError, exp errorType) { } return } - if got == nil && exp != errorNone { + if exp != errorNone { t.Fatalf("Expected error of type %q but got none", exp) } } diff --git a/web/federate.go b/web/federate.go index fa77cbd4f..a90f84374 100644 --- a/web/federate.go +++ b/web/federate.go @@ -136,7 +136,7 @@ func (h *Handler) federation(w http.ResponseWriter, req *http.Request) { } if set.Err() != nil { federationErrors.Inc() - http.Error(w, err.Error(), http.StatusInternalServerError) + http.Error(w, set.Err().Error(), http.StatusInternalServerError) return }