From 6687d6eac74d2d3055ccc5ec31209d079d6f1b03 Mon Sep 17 00:00:00 2001 From: Yuri Shkuro Date: Sat, 21 Oct 2023 23:07:35 -0400 Subject: [PATCH] test: Add more linters (#4881) ## Description of the changes - Enable more linters - Fix issues currently reported ## How was this change tested? - `make lint` --------- Signed-off-by: Yuri Shkuro --- .golangci.yml | 59 ++++++++++++++++++- cmd/internal/status/command.go | 7 ++- crossdock/main.go | 1 + internal/metricstest/local.go | 2 +- pkg/es/client/client.go | 1 + plugin/metrics/disabled/reader.go | 8 +-- .../strategystore/static/strategy_store.go | 11 +++- 7 files changed, 80 insertions(+), 9 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 0155fc92d70..a05cb1afc16 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -3,7 +3,10 @@ issues: exclude-rules: # Exclude some linters from running on tests files. - path: _test\.go - linters: [gosec] + linters: [gosec, bodyclose, noctx] + + - path: crossdock + linters: [noctx] # See https://github.com/jaegertracing/jaeger/issues/4488 - path: internal/grpctest/ @@ -22,16 +25,70 @@ linters: disable: - errcheck enable: + # Plain ASCII identifiers. + - asciicheck + + # Checks for dangerous unicode character sequences. - bidichk + + # Checks whether HTTP response body is closed successfully. + # TODO enable this but maybe find a way to disable in tests. + - bodyclose + + # Check whether the function uses a non-inherited context. - contextcheck + + # Check declaration order of types, consts, vars and funcs. + - decorder + + # Checks if package imports are in a list of acceptable packages. - depguard + + # Check for two durations multiplied together. + - durationcheck + + # Checks `Err-` prefix for var and `-Error` suffix for error type. + - errname + + # Suggests to use `%w` for error-wrapping. + # TODO enable this. Curently fails in about 20 places. + # - errorlint + + # Checks for pointers to enclosing loop variables. + - exportloopref + - gocritic + - gofmt - gofumpt - goimports + + # Allow or ban replace directives in go.mod + # or force explanation for retract directives. + # Maybe enable once we get rid of old sarama. + # - gomoddirectives + - gosec + + # Linter that specializes in simplifying code. + - gosimple - govet + + # Detects when assignments to existing variables are not used. + - ineffassign + - misspell + # Finds naked/bare returns and requires change them. + - nakedret + + # Require a bit more explicit returns. + - nilerr + + # Finds sending HTTP request without context.Context. + - noctx + + # TODO consider adding more linters, cf. https://olegk.dev/go-linters-configuration-the-right-version + linters-settings: depguard: list-type: blacklist diff --git a/cmd/internal/status/command.go b/cmd/internal/status/command.go index 5ea4f833f48..a3aef682f13 100644 --- a/cmd/internal/status/command.go +++ b/cmd/internal/status/command.go @@ -15,11 +15,13 @@ package status import ( + "context" "flag" "fmt" "io" "net/http" "strings" + "time" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -37,7 +39,10 @@ func Command(v *viper.Viper, adminPort int) *cobra.Command { Long: `Print Jaeger component status information, exit non-zero on any error.`, RunE: func(cmd *cobra.Command, args []string) error { url := convert(v.GetString(statusHTTPHostPort)) - resp, err := http.Get(url) + ctx, cx := context.WithTimeout(context.Background(), time.Second) + defer cx() + req, _ := http.NewRequestWithContext(ctx, "GET", url, nil) + resp, err := http.DefaultClient.Do(req) if err != nil { return err } diff --git a/crossdock/main.go b/crossdock/main.go index 3e36170f17a..163110c03fb 100644 --- a/crossdock/main.go +++ b/crossdock/main.go @@ -111,6 +111,7 @@ func is2xxStatusCode(statusCode int) bool { func httpHealthCheck(logger *zap.Logger, service, healthURL string) { for i := 0; i < 240; i++ { res, err := http.Get(healthURL) + res.Body.Close() if err == nil && is2xxStatusCode(res.StatusCode) { logger.Info("Health check successful", zap.String("service", service)) return diff --git a/internal/metricstest/local.go b/internal/metricstest/local.go index 4978a6d45b9..d725661bad3 100644 --- a/internal/metricstest/local.go +++ b/internal/metricstest/local.go @@ -251,7 +251,7 @@ func (b *Backend) Snapshot() (counters, gauges map[string]int64) { } } - return + return counters, gauges } // Stop cleanly closes the background goroutine spawned by NewBackend. diff --git a/pkg/es/client/client.go b/pkg/es/client/client.go index 911e24f2f57..7ea944e2862 100644 --- a/pkg/es/client/client.go +++ b/pkg/es/client/client.go @@ -87,6 +87,7 @@ func (c *Client) request(esRequest elasticRequest) ([]byte, error) { if err != nil { return []byte{}, err } + defer res.Body.Close() if res.StatusCode != http.StatusOK { return []byte{}, c.handleFailedRequest(res) diff --git a/plugin/metrics/disabled/reader.go b/plugin/metrics/disabled/reader.go index 8f8a3d9860e..c377eeb964e 100644 --- a/plugin/metrics/disabled/reader.go +++ b/plugin/metrics/disabled/reader.go @@ -27,14 +27,14 @@ type ( // the METRICS_STORAGE_TYPE has not been set. MetricsReader struct{} - // errMetricsQueryDisabled is the error returned by disabledMetricsQueryService. - errMetricsQueryDisabled struct{} + // errMetricsQueryDisabledError is the error returned by disabledMetricsQueryService. + errMetricsQueryDisabledError struct{} ) // ErrDisabled is the error returned by a "disabled" MetricsQueryService on all of its endpoints. -var ErrDisabled = &errMetricsQueryDisabled{} +var ErrDisabled = &errMetricsQueryDisabledError{} -func (m *errMetricsQueryDisabled) Error() string { +func (m *errMetricsQueryDisabledError) Error() string { return "metrics querying is currently disabled" } diff --git a/plugin/sampling/strategystore/static/strategy_store.go b/plugin/sampling/strategystore/static/strategy_store.go index 2c87d8073a6..cf8544630e9 100644 --- a/plugin/sampling/strategystore/static/strategy_store.go +++ b/plugin/sampling/strategystore/static/strategy_store.go @@ -97,12 +97,19 @@ func (h *strategyStore) Close() { func (h *strategyStore) downloadSamplingStrategies(url string) ([]byte, error) { h.logger.Info("Downloading sampling strategies", zap.String("url", url)) - resp, err := http.Get(url) + + ctx, cx := context.WithTimeout(context.Background(), time.Second) + defer cx() + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + if err != nil { + return nil, fmt.Errorf("cannot construct HTTP request: %w", err) + } + resp, err := http.DefaultClient.Do(req) if err != nil { return nil, fmt.Errorf("failed to download sampling strategies: %w", err) } - defer resp.Body.Close() + buf := new(bytes.Buffer) if _, err = buf.ReadFrom(resp.Body); err != nil { return nil, fmt.Errorf("failed to read sampling strategies HTTP response body: %w", err)