PIP-2108: Lint 4/4 (#132)

* PIP-2108: Enable linter `goimports`. * PIP-2108: Enable linter `gosec`. * PIP-2108: fix unit tests * PIP-2108: Enable linter `gosimple`. * PIP-2108: Enable linter `govet`. * PIP-2108: Enable linter `ineffassign`. * PIP-2108: Enable linter `staticcheck`. * PIP-2108: Enable linter `noctx`. * PIP-2108: Enable linters `typecheck`, `unused`. * PIP-2108: Enable linter `stylecheck`. * PIP-2108: Remove `go vet` from `make lint`.
mailgun · Oct 3, 2022 · 623b4c1 · 623b4c1
1 parent 19313a0
commit 623b4c1
Show file tree

Hide file tree

Showing 36 changed files with 171 additions and 171 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -8,17 +8,17 @@ linters:
     - depguard
     - errcheck # Mandatory. Do not disable.
     - gocritic
-    # - goimports
-    # - gosec
-    # - gosimple
-    # - govet
-    # - noctx
+    - goimports
+    - gosec
+    - gosimple
+    - govet
+    - noctx
     - nolintlint
-    # - ineffassign # Mandatory. Do not disable.
-    # - staticcheck # Mandatory. Do not disable.
-    # - stylecheck
-    # - typecheck
-    # - unused
+    - ineffassign # Mandatory. Do not disable.
+    - staticcheck # Mandatory. Do not disable.
+    - stylecheck
+    - typecheck
+    - unused
 
 # Other linters:
 #    - dogsled
@@ -105,6 +105,13 @@ issues:
   # Maximum count of issues with the same text. Set to 0 to disable. Default is 3.
   max-same-issues: 50
 
+  exclude-rules:
+    # Exclude some rules from tests.
+    - path: '_test\.go$'
+      linters:
+        - gosec
+        - noctx
+
 run:
   # include test files or not, default is true
   tests: true

diff --git a/Makefile b/Makefile
@@ -2,7 +2,6 @@ GOLINT = $(GOPATH)/bin/golangci-lint
 
 .PHONY: lint
 lint: $(GOLINT)
-	go vet ./...
 	$(GOLINT) run
 
 $(GOLINT):

diff --git a/anonymize/anonymize_test.go b/anonymize/anonymize_test.go
@@ -1,8 +1,9 @@
 package anonymize
 
 import (
-	"github.com/stretchr/testify/suite"
 	"testing"
+
+	"github.com/stretchr/testify/suite"
 )
 
 type AnonymizeSuite struct {

diff --git a/clock/system.go b/clock/system.go
@@ -59,7 +59,11 @@ func (st *systemTime) NewTicker(d time.Duration) Ticker {
 	return &systemTicker{t}
 }
 
+// Tick creates a new Ticker and returns ticker channel.
+// Use sparingly or in unit tests as this potentially generates a resource leak.
+// https://staticcheck.io/docs/checks#SA1015
 func (st *systemTime) Tick(d time.Duration) <-chan time.Time {
+	//nolint: staticcheck // FIXME: SA1015: using time.Tick leaks the underlying ticker, consider using it only in endless functions, tests and the main package, and use time.NewTicker here
 	return time.Tick(d)
 }
 

diff --git a/clock/system_test.go b/clock/system_test.go
@@ -66,7 +66,7 @@ func TestTimerStop(t *testing.T) {
 
 	// Then
 	assert.Equal(t, true, active)
-	time.Sleep(100)
+	time.Sleep(100 * time.Nanosecond)
 	select {
 	case <-timer.C():
 		assert.Fail(t, "Timer should not have fired")

diff --git a/cmd/election/main.go b/cmd/election/main.go
@@ -26,11 +26,10 @@ func sendRPC(ctx context.Context, peer string, req election.RPCRequest, resp *el
 	}
 
 	// Create a new http request with context
-	hr, err := http.NewRequest(http.MethodPost, fmt.Sprintf("http://%s/rpc", peer), bytes.NewBuffer(b))
+	hr, err := http.NewRequestWithContext(ctx, http.MethodPost, fmt.Sprintf("http://%s/rpc", peer), bytes.NewBuffer(b))
 	if err != nil {
 		return errors.Wrap(err, "while creating request")
 	}
-	hr.WithContext(ctx)
 
 	// Send the request
 	hp, err := http.DefaultClient.Do(hr)
@@ -134,7 +133,12 @@ func main() {
 	mux := http.NewServeMux()
 	mux.HandleFunc("/rpc", newHandler(node))
 	go func() {
-		logrus.Fatal(http.ListenAndServe(electionAddr, mux))
+		server := &http.Server{
+			Addr:              electionAddr,
+			Handler:           mux,
+			ReadHeaderTimeout: 1 * time.Minute,
+		}
+		logrus.Fatal(server.ListenAndServe())
 	}()
 
 	// Wait until the http server is up and can receive RPC requests

diff --git a/collections/ttlmap.go b/collections/ttlmap.go
@@ -99,7 +99,7 @@ func (m *TTLMap) Increment(key string, value, ttlSeconds int) (retval int, reter
 
 	currentValue, ok := mapEl.value.(int)
 	if !ok {
-		return 0, fmt.Errorf("Expected existing value to be integer, got %T", mapEl.value)
+		return 0, fmt.Errorf("Expected existing value to be integer, got %T", mapEl.value) //nolint:stylecheck // TODO(v5): ST1005: error strings should not be capitalized
 	}
 
 	currentValue += value
@@ -114,7 +114,7 @@ func (m *TTLMap) GetInt(key string) (retval int, found bool, reterr error) {
 	}
 	value, ok := valueI.(int)
 	if !ok {
-		return 0, false, fmt.Errorf("Expected existing value to be integer, got %T", valueI)
+		return 0, false, fmt.Errorf("Expected existing value to be integer, got %T", valueI) //nolint:stylecheck // TODO(v5): ST1005: error strings should not be capitalized
 	}
 	return value, true, nil
 }
@@ -140,7 +140,6 @@ func (m *TTLMap) set(key string, value interface{}, expiryTime int) {
 	heapEl.Value = mapEl
 	m.elements[key] = mapEl
 	m.expiryTimes.Push(heapEl)
-	return
 }
 
 func (m *TTLMap) lockNGet(key string) (value interface{}, mapEl *mapElement, expired bool) {

diff --git a/collections/ttlmap_test.go b/collections/ttlmap_test.go
@@ -109,10 +109,10 @@ func (s *TTLMapSuite) TestRemoveExpiredEdgeCase() {
 	err = m.Set("b", 2, 1)
 	s.Require().Equal(nil, err)
 
-	valI, exists := m.Get("a")
+	_, exists := m.Get("a")
 	s.Require().Equal(false, exists)
 
-	valI, exists = m.Get("b")
+	valI, exists := m.Get("b")
 	s.Require().Equal(true, exists)
 	s.Require().Equal(2, valI)
 
@@ -133,10 +133,10 @@ func (s *TTLMapSuite) TestRemoveOutOfCapacity() {
 	err = m.Set("c", 3, 10)
 	s.Require().Equal(nil, err)
 
-	valI, exists := m.Get("a")
+	_, exists := m.Get("a")
 	s.Require().Equal(false, exists)
 
-	valI, exists = m.Get("b")
+	valI, exists := m.Get("b")
 	s.Require().Equal(true, exists)
 	s.Require().Equal(2, valI)
 
@@ -233,7 +233,7 @@ func (s *TTLMapSuite) TestIncrementOutOfCapacity() {
 	s.Require().Equal(true, exists)
 	s.Require().Equal(4, val)
 
-	val, exists, err = m.GetInt("a")
+	_, exists, err = m.GetInt("a")
 
 	s.Require().Equal(nil, err)
 	s.Require().Equal(false, exists)
@@ -251,12 +251,12 @@ func (s *TTLMapSuite) TestIncrementRemovesExpired() {
 	_, err = m.Increment("c", 3, 3)
 	s.Require().NoError(err)
 
-	val, exists, err := m.GetInt("a")
+	_, exists, err := m.GetInt("a")
 
 	s.Require().Equal(nil, err)
 	s.Require().Equal(false, exists)
 
-	val, exists, err = m.GetInt("b")
+	val, exists, err := m.GetInt("b")
 	s.Require().Equal(nil, err)
 	s.Require().Equal(true, exists)
 	s.Require().Equal(2, val)
@@ -277,12 +277,12 @@ func (s *TTLMapSuite) TestIncrementRemovesLastUsed() {
 	_, err = m.Increment("c", 3, 12)
 	s.Require().NoError(err)
 
-	val, exists, err := m.GetInt("a")
+	_, exists, err := m.GetInt("a")
 
 	s.Require().Equal(nil, err)
 	s.Require().Equal(false, exists)
 
-	val, exists, err = m.GetInt("b")
+	val, exists, err := m.GetInt("b")
 	s.Require().Equal(nil, err)
 	s.Require().Equal(true, exists)
 

diff --git a/consul/config_test.go b/consul/config_test.go
@@ -25,6 +25,7 @@ func TestNewClientTLS(t *testing.T) {
 	_, err = client.KV().Put(&kv, nil)
 	require.NoError(t, err)
 	resp, _, err := client.KV().Get("test-key-tls", nil)
+	require.NoError(t, err)
 	assert.Equal(t, resp.Key, "test-key-tls")
 	assert.Equal(t, resp.Value, []byte("test-value-tls"))
 }
@@ -40,6 +41,7 @@ func TestNewClient(t *testing.T) {
 	_, err = client.KV().Put(&kv, nil)
 	require.NoError(t, err)
 	resp, _, err := client.KV().Get("test-key", nil)
+	require.NoError(t, err)
 	assert.Equal(t, resp.Key, "test-key")
 	assert.Equal(t, resp.Value, []byte("test-value"))
 }

diff --git a/consul/lock_test.go b/consul/lock_test.go
@@ -229,6 +229,7 @@ func TestBehaviorDeleteOnDisconnect(t *testing.T) {
 
 		// Wait for the lock file to disappear
 		client, err := api.NewClient(api.DefaultConfig())
+		require.NoError(t, err)
 		testutil.UntilPass(t, 50, time.Second, func(t testutil.TestingT) {
 			kv, _, err := client.KV().Get(name, nil)
 			assert.NoError(t, err)

diff --git a/discovery/consul_test.go b/discovery/consul_test.go
@@ -2,35 +2,20 @@ package discovery_test
 
 import (
 	"context"
-	"fmt"
 	"testing"
 
-	"github.com/hashicorp/consul/api"
 	"github.com/mailgun/holster/v4/discovery"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func printCatalog(t *testing.T, catalog string, client *api.Client) {
-	t.Helper()
-	fmt.Printf("============\n")
-	l, _, err := client.Health().Service(catalog, "", false, nil)
-	require.NoError(t, err)
-	for _, i := range l {
-		t.Logf("Service: %s", i.Service.ID)
-	}
-	fmt.Printf("======\n")
-}
-
 func TestConsulSinglePeer(t *testing.T) {
 	const catalog = "TestConsulSinglePeer"
 	p := discovery.Peer{ID: "id-1", Metadata: []byte("address-0"), IsSelf: true}
 
 	// client, err := api.NewClient(api.DefaultConfig())
 	// require.NoError(t, err)
 
-	// printCatalog(t, catalog, client)
-
 	onUpdateCh := make(chan []discovery.Peer, 1)
 	cs, err := discovery.NewConsul(&discovery.ConsulConfig{
 		CatalogName: catalog,
@@ -39,15 +24,13 @@ func TestConsulSinglePeer(t *testing.T) {
 			onUpdateCh <- peers
 		},
 	})
-	// printCatalog(t, catalog, client)
+	require.NoError(t, err)
 
 	e := <-onUpdateCh
 	assert.Equal(t, p, e[0])
 
 	err = cs.Close(context.Background())
 	require.NoError(t, err)
-
-	// printCatalog(t, catalog, client)
 }
 
 func TestConsulMultiplePeers(t *testing.T) {
@@ -58,8 +41,6 @@ func TestConsulMultiplePeers(t *testing.T) {
 	// client, err := api.NewClient(api.DefaultConfig())
 	// require.NoError(t, err)
 
-	// printCatalog(t, catalog, client)
-
 	onUpdateCh := make(chan []discovery.Peer, 2)
 	cs0, err := discovery.NewConsul(&discovery.ConsulConfig{
 		CatalogName: catalog,
@@ -74,8 +55,6 @@ func TestConsulMultiplePeers(t *testing.T) {
 	e := <-onUpdateCh
 	assert.Equal(t, e[0], p0)
 
-	// printCatalog(t, catalog, client)
-
 	cs1, err := discovery.NewConsul(&discovery.ConsulConfig{
 		CatalogName: catalog,
 		Peer:        p1,
@@ -85,6 +64,4 @@ func TestConsulMultiplePeers(t *testing.T) {
 
 	e = <-onUpdateCh
 	assert.Equal(t, []discovery.Peer{p0, p1}, e)
-
-	// printCatalog(t, catalog, client)
 }
diff --git a/discovery/memberlist.go b/discovery/memberlist.go
@@ -44,7 +44,6 @@ type MemberList struct {
 	memberList *ml.Memberlist
 	conf       MemberListConfig
 	events     *eventDelegate
-	mutex      sync.Mutex
 }
 
 type MemberListConfig struct {
@@ -239,7 +238,7 @@ func NewLogWriter(log logrus.FieldLogger) *io.PipeWriter {
 		reader.Close()
 	}()
 	runtime.SetFinalizer(writer, func(w *io.PipeWriter) {
-		writer.Close()
+		w.Close()
 	})
 
 	return writer

diff --git a/election/cluster_test.go b/election/cluster_test.go
@@ -95,7 +95,7 @@ func (c *TestCluster) Remove(name string) *ClusterNode {
 func (c *TestCluster) updatePeers() {
 	// Build a list of all the peers
 	var peers []string
-	for k, _ := range c.Nodes {
+	for k := range c.Nodes {
 		peers = append(peers, k)
 	}
 

diff --git a/election/election.go b/election/election.go
@@ -322,10 +322,9 @@ func (e *node) Stop(ctx context.Context) error {
 		e.wg.Wait()
 		close(done)
 	}()
-	select {
-	case <-done:
-		return ctx.Err()
-	}
+
+	<-done
+	return ctx.Err()
 }
 
 // Main thread loop
@@ -363,7 +362,7 @@ func (e *node) runFollower() {
 			e.processRPC(rpc)
 		case <-heartbeatTimer.C:
 			// Check if we have had successful contact with the leader
-			if time.Now().Sub(e.lastContact) < e.conf.HeartBeatTimeout {
+			if time.Since(e.lastContact) < e.conf.HeartBeatTimeout {
 				continue
 			}
 
@@ -816,7 +815,6 @@ func (e *node) handleVote(rpc RPCRequest, req VoteReq) {
 	// Tell the requester we voted for him
 	resp.Granted = true
 	e.lastContact = time.Now()
-	return
 }
 
 func (e *node) handleSetPeers(rpc RPCRequest, req SetPeersReq) {
@@ -861,7 +859,7 @@ func (e *node) send(req interface{}) chan RPCResponse {
 
 // randomDuration returns a value that is between the minDur and 2x minDur.
 func randomDuration(minDur time.Duration) time.Duration {
-	return minDur + time.Duration(rand.Int63())%minDur
+	return minDur + time.Duration(rand.Int63())%minDur //nolint:gosec // Cryptographic security not required.
 }
 
 // WaitForConnect waits for the specified address to accept connections then returns nil.

diff --git a/election/election_test.go b/election/election_test.go
@@ -409,7 +409,7 @@ func TestResign(t *testing.T) {
 
 	// Calling resign on a follower should have no effect
 	err := c1.Nodes["n1"].Node.Resign(context.Background())
-	require.NoError(t, err)
+	assert.ErrorContains(t, err, "not the leader")
 
 	for i := 0; i < 10; i++ {
 		if c1.GetLeader() != leader {