Enable revive linter (jaegertracing#5505)

## Which problem is this PR solving? - revive is a pretty comprehensive linter that catches many issues - incidentally, I wanted to have a linter for the change in jaegertracing#5377, but the `errorf` [rule in revive](https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md#errorf) is only catching `errors.New(fmt.Sprintf())`, not `fmt.Errorf(just_string)` scenarios. ## Description of the changes - add revive to the main list of linters - disable some of its rules which are currently breaking for future clean-up - clean-up a few places what had low number of breaks ## How was this change tested? - `make lint` Signed-off-by: Yuri Shkuro <[email protected]>
FlamingSaint · Jun 2, 2024 · b38d2f9 · b38d2f9
1 parent ca50f53
commit b38d2f9
Show file tree

Hide file tree

Showing 9 changed files with 137 additions and 13 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -86,6 +86,9 @@ linters:
     # Finds sending HTTP request without context.Context.
     - noctx
 
+    # Fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in replacement of golint.
+    - revive
+
     # Checks usage of github.com/stretchr/testify.
     - testifylint
 
@@ -112,7 +115,7 @@ linters-settings:
             desc: "Do not refer to crossdock from other packages"
         files:
           - "!**/crossdock/**"
-      
+
       disallow-uber/goleak:
         deny:
          - pkg: go.uber.org/goleak
@@ -143,6 +146,118 @@ linters-settings:
       - fieldalignment
       # Disable shadow
       - shadow
+  revive:
+      ignore-generated-header: true
+      severity: error
+      enable-all-rules: true
+      # See https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md
+      rules:
+        # not a completely bad linter, but needs clean-up and sensible width (80 is too small)
+        - name: line-length-limit
+          disabled: true
+          arguments: [80]
+        # would be ok if we could exclude the test files, but otherwise too noisy
+        - name: add-constant
+          disabled: true
+        # maybe enable in the future, needs more investigation
+        - name: cognitive-complexity
+          disabled: true
+        # not sure how different from previous one
+        - name: cyclomatic
+          disabled: true
+        # do a clean-up and enable
+        - name: use-any
+          disabled: true
+        # do a clean-up and enable
+        - name: unused-parameter
+          disabled: true
+        # do a clean-up and enable
+        - name: unused-receiver
+          disabled: true
+        # we use storage_v2, so...
+        - name: var-naming
+          disabled: true
+        # could be useful to catch issues, but needs a clean-up and some ignores
+        - name: unchecked-type-assertion
+          disabled: true
+        # wtf: "you have exceeded the maximum number of public struct declarations"
+        - name: max-public-structs
+          disabled: true
+        # probably a good one to enable after cleanup
+        - name: import-shadowing
+          disabled: true
+        # TBD - often triggered in tests
+        - name: unhandled-error
+          disabled: true
+        # this one looks like it's catching real errors, need to enable it
+        - name: modifies-value-receiver
+          disabled: true
+        # often looks like a red herring, needs investigation
+        - name: flag-parameter
+          disabled: true
+        # looks like a good linter, needs cleanup
+        - name: confusing-naming
+          disabled: true
+        # too pendantic
+        - name: function-length
+          disabled: true
+        # definitely a good one, needs cleanup first
+        - name: argument-limit
+          disabled: true
+        # another good one, needs cleanup first
+        - name: unexported-naming
+          disabled: true
+        # maybe enable, needs invesitgation of the impact
+        - name: import-alias-naming
+          disabled: true
+        # maybe enable, needs invesitgation of the impact
+        - name: get-return
+          disabled: true
+        # enable after cleanup
+        - name: early-return
+          disabled: true
+        # enable after cleanup
+        - name: bare-return
+          disabled: true
+        # enable after cleanup
+        - name: empty-lines
+          disabled: true
+        # investigate, could be real bugs. But didn't recent Go version changed loop variables semantics?
+        - name: range-val-address
+          disabled: true
+        # enable after cleanup
+        - name: confusing-results
+          disabled: true
+        # we do use dot imports, but not a bad idea to make it explicit
+        - name: dot-imports
+          disabled: true
+        # enable after cleanup: "tag on not-exported field"
+        - name: struct-tag
+          disabled: true
+        # enable after cleanup
+        - name: receiver-naming
+          disabled: true
+        # this is idiocy, promotes less readable code. Don't enable.
+        - name: var-declaration
+          disabled: true
+        # enable after cleanup: warns of stutter names
+        - name: exported
+          disabled: true
+        # enable after cleanup
+        - name: redefines-builtin-id
+          disabled: true
+        # "no nested structs are allowed" - don't enable, doesn't make sense
+        - name: nested-structs
+          disabled: true
+        # enable after cleanup
+        - name: indent-error-flow
+          disabled: true
+        # enable after cleanup
+        - name: unexported-return
+          disabled: true
+        # looks useful, but requires refactoring: "calls to log.Fatal only in main() or init() functions"
+        - name: deep-exit
+          disabled: true
   testifylint:
     disable:
       - float-compare

diff --git a/cmd/agent/app/processors/thrift_processor_test.go b/cmd/agent/app/processors/thrift_processor_test.go
@@ -79,6 +79,7 @@ func createProcessor(t *testing.T, mFactory metrics.Factory, tFactory thrift.TPr
 	return transport.Addr().String(), processor
 }
 
+// revive:disable-next-line function-result-limit
 func initCollectorAndReporter(t *testing.T) (*metricstest.Factory, *testutils.GrpcCollector, reporter.Reporter, *grpc.ClientConn) {
 	grpcCollector := testutils.StartGRPCCollector(t)
 	conn, err := grpc.NewClient(grpcCollector.Listener().Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()))

diff --git a/cmd/agent/app/reporter/metrics_test.go b/cmd/agent/app/reporter/metrics_test.go
@@ -101,11 +101,13 @@ func TestMetricsReporter(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		metricsFactory := metricstest.NewFactory(time.Microsecond)
-		defer metricsFactory.Stop()
-		r := WrapWithMetrics(test.rep, metricsFactory)
-		test.action(r)
-		metricsFactory.AssertCounterMetrics(t, test.expectedCounters...)
-		metricsFactory.AssertGaugeMetrics(t, test.expectedGauges...)
+		t.Run("", func(t *testing.T) {
+			metricsFactory := metricstest.NewFactory(time.Microsecond)
+			defer metricsFactory.Stop()
+			r := WrapWithMetrics(test.rep, metricsFactory)
+			test.action(r)
+			metricsFactory.AssertCounterMetrics(t, test.expectedCounters...)
+			metricsFactory.AssertGaugeMetrics(t, test.expectedGauges...)
+		})
 	}
 }
diff --git a/cmd/collector/app/span_processor_test.go b/cmd/collector/app/span_processor_test.go
@@ -81,7 +81,7 @@ func TestBySvcMetrics(t *testing.T) {
 		}
 	}
 
-	for _, test := range tests {
+	testFn := func(t *testing.T, test TestCase) {
 		mb := metricstest.NewFactory(time.Hour)
 		defer mb.Backend.Stop()
 		logger := zap.NewNop()
@@ -160,6 +160,11 @@ func TestBySvcMetrics(t *testing.T) {
 		}
 		mb.AssertCounterMetrics(t, expected...)
 	}
+	for _, test := range tests {
+		t.Run(fmt.Sprintf("%v", test), func(t *testing.T) {
+			testFn(t, test)
+		})
+	}
 }
 
 func isSpanAllowed(span *model.Span) bool {

diff --git a/cmd/ingester/app/consumer/consumer.go b/cmd/ingester/app/consumer/consumer.go
@@ -145,6 +145,7 @@ func (c *Consumer) handleMessages(pc sc.PartitionConsumer) {
 
 			if msgProcessor == nil {
 				msgProcessor = c.processorFactory.new(pc.Topic(), pc.Partition(), msg.Offset-1)
+				// revive:disable-next-line defer
 				defer msgProcessor.Close()
 			}
 

diff --git a/cmd/internal/flags/service_test.go b/cmd/internal/flags/service_test.go
@@ -74,7 +74,7 @@ func TestStartErrors(t *testing.T) {
 	}
 	for _, test := range scenarios {
 		t.Run(test.name, func(t *testing.T) {
-			s := NewService( /*default port=*/ 0)
+			s := NewService( /* default port= */ 0)
 			v, cmd := config.Viperize(s.AddFlags)
 			err := cmd.ParseFlags(test.flags)
 			require.NoError(t, err)

diff --git a/cmd/jaeger/internal/extension/jaegerstorage/extension.go b/cmd/jaeger/internal/extension/jaegerstorage/extension.go
@@ -17,7 +17,6 @@ import (
 	memoryCfg "github.com/jaegertracing/jaeger/pkg/memory/config"
 	"github.com/jaegertracing/jaeger/pkg/metrics"
 	"github.com/jaegertracing/jaeger/plugin/storage/badger"
-	badgerCfg "github.com/jaegertracing/jaeger/plugin/storage/badger"
 	"github.com/jaegertracing/jaeger/plugin/storage/cassandra"
 	"github.com/jaegertracing/jaeger/plugin/storage/es"
 	"github.com/jaegertracing/jaeger/plugin/storage/grpc"
@@ -79,7 +78,7 @@ type starter[Config any, Factory storage.Factory] struct {
 	builder     func(Config, metrics.Factory, *zap.Logger) (Factory, error)
 }
 
-func (s *starter[Config, Factory]) build(ctx context.Context, host component.Host) error {
+func (s *starter[Config, Factory]) build(_ context.Context, _ component.Host) error {
 	for name, cfg := range s.cfg {
 		if _, ok := s.ext.factories[name]; ok {
 			return fmt.Errorf("duplicate %s storage name %s", s.storageKind, name)
@@ -111,7 +110,7 @@ func (s *storageExt) Start(ctx context.Context, host component.Host) error {
 			return memory.NewFactoryWithConfig(cfg, metricsFactory, logger), nil
 		},
 	}
-	badgerStarter := &starter[badgerCfg.NamespaceConfig, *badger.Factory]{
+	badgerStarter := &starter[badger.NamespaceConfig, *badger.Factory]{
 		ext:         s,
 		storageKind: "badger",
 		cfg:         s.config.Badger,

diff --git a/cmd/query/app/grpc_handler_test.go b/cmd/query/app/grpc_handler_test.go
@@ -948,6 +948,7 @@ func withTenantedServerAndClient(t *testing.T, tm *tenancy.Manager, actualTest f
 }
 
 // withOutgoingMetadata returns a Context with metadata for a server to receive
+// revive:disable-next-line context-as-argument
 func withOutgoingMetadata(t *testing.T, ctx context.Context, headerName, headerValue string) context.Context {
 	t.Helper()
 

diff --git a/cmd/query/app/internal/api_v3/traces.go b/cmd/query/app/internal/api_v3/traces.go
@@ -5,7 +5,7 @@ package api_v3
 
 import (
 	"github.com/gogo/protobuf/jsonpb"
-	proto "github.com/gogo/protobuf/proto"
+	"github.com/gogo/protobuf/proto"
 	"go.opentelemetry.io/collector/pdata/ptrace"
 
 	"github.com/jaegertracing/jaeger/pkg/gogocodec"