Add staticcheck github action, resolve existing lint (#254)

This commit adds staticcheck as a github action and resolves existing issues flagged by staticcheck. I've grouped the issues into individual commits so that they can be considered independently. Most of the issues are deprecated uses of stdlib, unused errors, and ineffective branches or statements. But, there is one stylistic issue too that is more subjective.

staticcheck is a very conservative linter that has a focus on minimizing false positives. I've found that staticcheck has improved the quality of my own work, and has also made pull requests easier to evaluate.

Each issue is tagged with an identifier that can be consulted here: https://staticcheck.dev/docs/checks/

.github/workflows/static-check.yml

@@ -0,0 +1,26 @@
+name: static-check
+
+on:
+  pull_request:
+  push:
+    tags:
+      - 'v*'
+    branches:
+      - main
+
+jobs:
+  staticcheck:
+    name: staticcheck (project)
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v1
+        with:
+          fetch-depth: 1
+      - id: install_go
+        uses: WillAbides/[email protected]
+        with:
+          go-version: "1.20.x"
+      - uses: dominikh/[email protected]
+        with:
+          install-go: false
+          version: "2023.1.7"

client/httpclient_test.go

@@ -3,7 +3,7 @@ package client
 import (
 	"compress/gzip"
 	"context"
-	"io/ioutil"
+	"io"
 	"net/http"
 	"sync/atomic"
 	"testing"
@@ -22,10 +22,12 @@ func TestHTTPPolling(t *testing.T) {
 	srv := internal.StartMockServer(t)
 	var rcvCounter int64
 	srv.OnMessage = func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
-		assert.EqualValues(t, rcvCounter, msg.SequenceNum)
-		if msg != nil {
-			atomic.AddInt64(&rcvCounter, 1)
+		if msg == nil {
+			t.Error("unexpected nil msg")
+			return nil
 		}
+		assert.EqualValues(t, rcvCounter, msg.SequenceNum)
+		atomic.AddInt64(&rcvCounter, 1)
 		return nil
 	}
 
@@ -63,7 +65,7 @@ func TestHTTPClientCompression(t *testing.T) {
 		assert.Equal(t, "gzip", r.Header.Get("Content-Encoding"))
 		reader, err := gzip.NewReader(r.Body)
 		assert.NoError(t, err)
-		body, err := ioutil.ReadAll(reader)
+		body, err := io.ReadAll(reader)
 		assert.NoError(t, err)
 		_ = r.Body.Close()
 		var response protobufs.AgentToServer
@@ -112,10 +114,12 @@ func TestHTTPClientSetPollingInterval(t *testing.T) {
 	srv := internal.StartMockServer(t)
 	var rcvCounter int64
 	srv.OnMessage = func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
-		assert.EqualValues(t, rcvCounter, msg.SequenceNum)
-		if msg != nil {
-			atomic.AddInt64(&rcvCounter, 1)
+		if msg == nil {
+			t.Error("unexpected nil msg")
+			return nil
 		}
+		assert.EqualValues(t, rcvCounter, msg.SequenceNum)
+		atomic.AddInt64(&rcvCounter, 1)
 		return nil
 	}
 

client/internal/httpsender.go

@@ -110,7 +110,6 @@ func (h *HTTPSender) Run(
 			// This will make hasPendingMessage channel readable, so we will enter
 			// the case above on the next iteration of the loop.
 			h.ScheduleSend()
-			break
 
 		case <-ctx.Done():
 			return

client/internal/mockserver.go

@@ -40,8 +40,8 @@ const contentTypeProtobuf = "application/x-protobuf"
 func newMockServer(t *testing.T) (*MockServer, *http.ServeMux) {
 	srv := &MockServer{
 		t:                t,
-		expectedHandlers: make(chan receivedMessageHandler, 0),
-		expectedComplete: make(chan struct{}, 0),
+		expectedHandlers: make(chan receivedMessageHandler),
+		expectedComplete: make(chan struct{}),
 	}
 
 	m := http.NewServeMux()
@@ -112,6 +112,11 @@ func (m *MockServer) EnableExpectMode() {
 
 func (m *MockServer) handlePlainHttp(w http.ResponseWriter, r *http.Request) {
 	msgBytes, err := io.ReadAll(r.Body)
+	if err != nil {
+		// request could not be read for some reason
+		http.Error(w, err.Error(), 500)
+		return
+	}
 
 	// We use alwaysRespond=true here because plain HTTP requests must always have
 	// a response.

client/internal/packagessyncer.go

@@ -104,7 +104,7 @@ func (s *packagesSyncer) doSync(ctx context.Context) {
 		s.logger.Errorf(ctx, "Package syncing failed: %V", err)
 		return
 	}
-	if bytes.Compare(hash, s.available.AllPackagesHash) == 0 {
+	if bytes.Equal(hash, s.available.AllPackagesHash) {
 		s.logger.Debugf(ctx, "All packages are already up to date.")
 		return
 	}
@@ -243,7 +243,7 @@ func (s *packagesSyncer) shouldDownloadFile(ctx context.Context, packageName str
 	} else {
 		// Compare the checksum of the file we have with what
 		// we are offered by the server.
-		if bytes.Compare(fileContentHash, file.ContentHash) != 0 {
+		if !bytes.Equal(fileContentHash, file.ContentHash) {
 			s.logger.Debugf(ctx, "Package %s: file hash mismatch, will download.", packageName)
 			return true, nil
 		}

client/wsclient.go

@@ -128,7 +128,7 @@ func (c *wsClient) SetPackageStatuses(statuses *protobufs.PackageStatuses) error
 // Try to connect once. Returns an error if connection fails and optional retryAfter
 // duration to indicate to the caller to retry after the specified time as instructed
 // by the Server.
-func (c *wsClient) tryConnectOnce(ctx context.Context) (err error, retryAfter sharedinternal.OptionalDuration) {
+func (c *wsClient) tryConnectOnce(ctx context.Context) (retryAfter sharedinternal.OptionalDuration, err error) {
 	var resp *http.Response
 	conn, resp, err := c.dialer.DialContext(ctx, c.url.String(), c.requestHeader)
 	if err != nil {
@@ -142,7 +142,7 @@ func (c *wsClient) tryConnectOnce(ctx context.Context) (err error, retryAfter sh
 				redirect, err := resp.Location()
 				if err != nil {
 					c.common.Logger.Errorf(ctx, "%d redirect, but no valid location: %s", resp.StatusCode, err)
-					return err, duration
+					return duration, err
 				}
 				// rewrite the scheme for the sake of tolerance
 				if redirect.Scheme == "http" {
@@ -157,9 +157,9 @@ func (c *wsClient) tryConnectOnce(ctx context.Context) (err error, retryAfter sh
 			} else {
 				c.common.Logger.Errorf(ctx, "Server responded with status=%v", resp.Status)
 			}
-			return err, duration
+			return duration, err
 		}
-		return err, sharedinternal.OptionalDuration{Defined: false}
+		return sharedinternal.OptionalDuration{Defined: false}, err
 	}
 
 	// Successfully connected.
@@ -170,7 +170,7 @@ func (c *wsClient) tryConnectOnce(ctx context.Context) (err error, retryAfter sh
 		c.common.Callbacks.OnConnect(ctx)
 	}
 
-	return nil, sharedinternal.OptionalDuration{Defined: false}
+	return sharedinternal.OptionalDuration{Defined: false}, nil
 }
 
 // Continuously try until connected. Will return nil when successfully
@@ -190,7 +190,7 @@ func (c *wsClient) ensureConnected(ctx context.Context) error {
 		select {
 		case <-timer.C:
 			{
-				if err, retryAfter := c.tryConnectOnce(ctx); err != nil {
+				if retryAfter, err := c.tryConnectOnce(ctx); err != nil {
 					c.lastInternalErr.Store(&err)
 					if errors.Is(err, context.Canceled) {
 						c.common.Logger.Debugf(ctx, "Client is stopped, will not try anymore.")

internal/certs.go

@@ -15,7 +15,6 @@ import (
 	"encoding/pem"
 	"errors"
 	"fmt"
-	"io/ioutil"
 	"math/big"
 	"net"
 	"os"
@@ -79,19 +78,18 @@ func CreateServerTLSConfig(caCertPath, serverCertPath, serverKeyPath string) (*t
 		InsecureSkipVerify: true,
 		ClientCAs:          caCertPool,
 	}
-	tlsConfig.BuildNameToCertificate()
 	return tlsConfig, nil
 }
 
 func CreateTLSCert(caCertPath, caKeyPath string) (*protobufs.TLSCertificate, error) {
 
 	// Load CA Cert.
-	caCertBytes, err := ioutil.ReadFile(caCertPath)
+	caCertBytes, err := os.ReadFile(caCertPath)
 	if err != nil {
 		return nil, fmt.Errorf("cannot read CA cert: %v", err)
 	}
 
-	caKeyBytes, err := ioutil.ReadFile(caKeyPath)
+	caKeyBytes, err := os.ReadFile(caKeyPath)
 	if err != nil {
 		return nil, fmt.Errorf("cannot read CA key: %v", err)
 	}

internal/retryafter_test.go

@@ -1,12 +1,13 @@
 package internal
 
 import (
-	"github.com/stretchr/testify/assert"
 	"math/rand"
 	"net/http"
 	"strconv"
 	"testing"
 	"time"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func response503() *http.Response {
@@ -32,7 +33,6 @@ func assertDuration(t *testing.T, duration OptionalDuration, expected time.Durat
 
 func TestExtractRetryAfterHeaderDelaySeconds(t *testing.T) {
 	// Generate random n > 0 int
-	rand.Seed(time.Now().UnixNano())
 	retryIntervalSec := rand.Intn(9999)
 
 	// Generate a 503 status code response with Retry-After = n header
@@ -58,7 +58,6 @@ func TestExtractRetryAfterHeaderDelaySeconds(t *testing.T) {
 func TestExtractRetryAfterHeaderHttpDate(t *testing.T) {
 	// Generate a random n > 0 second duration
 	now := time.Now()
-	rand.Seed(now.UnixNano())
 	retryIntervalSec := rand.Intn(9999)
 	expectedDuration := time.Second * time.Duration(retryIntervalSec)
 

internal/wsmessage.go

@@ -11,6 +11,7 @@ import (
 // Message header is currently uint64 zero value.
 const wsMsgHeader = uint64(0)
 
+// DecodeWSMessage decodes a websocket message as bytes into a proto.Message.
 func DecodeWSMessage(bytes []byte, msg proto.Message) error {
 	// Message header is optional until the end of grace period that ends Feb 1, 2023.
 	// Check if the header is present.
@@ -23,9 +24,9 @@ func DecodeWSMessage(bytes []byte, msg proto.Message) error {
 		}
 		// Skip the header. It really is just a single zero byte for now.
 		bytes = bytes[n:]
-	} else {
-		// Old message format. No header present.
 	}
+	// If no header was present (the "if" check above), then this is the old
+	// message format. No header is present.
 
 	// Decode WebSocket message as a Protobuf message.
 	err := proto.Unmarshal(bytes, msg)

protobufshelpers/anyvaluehelpers.go

@@ -39,7 +39,7 @@ func IsEqualAnyValue(v1, v2 *protobufs.AnyValue) bool {
 
 	case *protobufs.AnyValue_BytesValue:
 		v2, ok := v2.Value.(*protobufs.AnyValue_BytesValue)
-		return ok && bytes.Compare(v1.BytesValue, v2.BytesValue) == 0
+		return ok && bytes.Equal(v1.BytesValue, v2.BytesValue)
 
 	case *protobufs.AnyValue_ArrayValue:
 		v2, ok := v2.Value.(*protobufs.AnyValue_ArrayValue)

server/serverimpl_test.go

@@ -628,6 +628,7 @@ func TestServerHonoursClientRequestContentEncoding(t *testing.T) {
 	require.NoError(t, err)
 
 	req, err := http.NewRequest("POST", "http://"+settings.ListenEndpoint+settings.ListenPath, bytes.NewReader(b))
+	require.NoError(t, err)
 	req.Header.Set(headerContentType, contentTypeProtobuf)
 	req.Header.Set(headerContentEncoding, contentEncodingGzip)
 	resp, err := hc.Do(req)
@@ -700,6 +701,7 @@ func TestServerHonoursAcceptEncoding(t *testing.T) {
 	b, err := proto.Marshal(&sendMsg)
 	require.NoError(t, err)
 	req, err := http.NewRequest("POST", "http://"+settings.ListenEndpoint+settings.ListenPath, bytes.NewReader(b))
+	require.NoError(t, err)
 	req.Header.Set(headerContentType, contentTypeProtobuf)
 	req.Header.Set(headerAcceptEncoding, contentEncodingGzip)
 	resp, err := hc.Do(req)