ci: enable SA4006 staticcheck check

And fix the 'value not used' issues.

Many of these are not bugs, but a few are tests not checking errors, and
one appears to be a missed error in non-test code.

.golangci.yml

@@ -17,10 +17,6 @@ issues:
     - linters: [staticcheck]
       text: "SA9004:"
 
-    # Temp ignore SA4006: this value of `x` is never used
-    - linters: [staticcheck]
-      text: "SA4006:"
-
 linters-settings:
   gofmt:
     simplify: false

agent/cache-types/catalog_datacenters_test.go

@@ -54,33 +54,28 @@ func TestCatalogDatacenters(t *testing.T) {
 
 	// Fetch first time
 	result, err := typ.Fetch(cache.FetchOptions{}, &structs.DatacentersRequest{})
-	result2, err := typ.Fetch(cache.FetchOptions{LastResult: &result}, &structs.DatacentersRequest{QueryOptions: structs.QueryOptions{MustRevalidate: true}})
-	result3, err := typ.Fetch(cache.FetchOptions{LastResult: &result2}, &structs.DatacentersRequest{QueryOptions: structs.QueryOptions{MustRevalidate: true}})
-
-	// make sure it was called the right number of times
-	rpc.AssertExpectations(t)
-
-	// make sure the first result was correct
 	require.NoError(t, err)
 	require.Equal(t, result, cache.FetchResult{
 		Value: resp,
 		Index: 1,
 	})
 
-	// validate the second result
+	result2, err := typ.Fetch(cache.FetchOptions{LastResult: &result}, &structs.DatacentersRequest{QueryOptions: structs.QueryOptions{MustRevalidate: true}})
 	require.NoError(t, err)
 	require.Equal(t, result2, cache.FetchResult{
 		Value: resp2,
 		Index: 2,
 	})
 
-	// validate the third result
+	result3, err := typ.Fetch(cache.FetchOptions{LastResult: &result2}, &structs.DatacentersRequest{QueryOptions: structs.QueryOptions{MustRevalidate: true}})
 	require.NoError(t, err)
 	require.Equal(t, result3, cache.FetchResult{
 		Value: resp3,
 		Index: 3,
 	})
 
+	// make sure it was called the right number of times
+	rpc.AssertExpectations(t)
 }
 
 func TestDatacenters_badReqType(t *testing.T) {

agent/cache-types/service_checks.go

@@ -76,6 +76,9 @@ func (c *ServiceHTTPChecks) Fetch(opts cache.FetchOptions, req cache.Request) (c
 			return hash, reply, nil
 		},
 	)
+	if err != nil {
+		return result, err
+	}
 
 	result.Value = resp
 

agent/consul/acl_endpoint_test.go

@@ -85,6 +85,7 @@ func TestACLEndpoint_BootstrapTokens(t *testing.T) {
 	require.True(t, strings.HasPrefix(err.Error(), structs.ACLBootstrapNotAllowedErr.Error()))
 
 	_, resetIdx, err := srv.fsm.State().CanBootstrapACLToken()
+	require.NoError(t, err)
 
 	resetPath := filepath.Join(dir, "acl-bootstrap-reset")
 	require.NoError(t, ioutil.WriteFile(resetPath, []byte(fmt.Sprintf("%d", resetIdx)), 0600))
@@ -1692,6 +1693,7 @@ func TestACLEndpoint_TokenSet_anon(t *testing.T) {
 	require.NotEmpty(t, token.SecretID)
 
 	tokenResp, err := retrieveTestToken(codec, TestDefaultMasterToken, "dc1", structs.ACLTokenAnonymousID)
+	require.NoError(t, err)
 	require.Equal(t, len(tokenResp.Token.Policies), 1)
 	require.Equal(t, tokenResp.Token.Policies[0].ID, policy.ID)
 
@@ -1900,6 +1902,7 @@ func TestACLEndpoint_TokenDelete_anon(t *testing.T) {
 
 	// Make sure the token is still there
 	tokenResp, err := retrieveTestToken(codec, TestDefaultMasterToken, "dc1", structs.ACLTokenAnonymousID)
+	require.NoError(t, err)
 	require.NotNil(t, tokenResp.Token)
 }
 
@@ -2291,6 +2294,7 @@ func TestACLEndpoint_PolicyDelete(t *testing.T) {
 
 	// Make sure the policy is gone
 	tokenResp, err := retrieveTestPolicy(codec, TestDefaultMasterToken, "dc1", existingPolicy.ID)
+	require.NoError(t, err)
 	require.Nil(t, tokenResp.Policy)
 }
 
@@ -2903,6 +2907,7 @@ func TestACLEndpoint_RoleDelete(t *testing.T) {
 
 	// Make sure the role is gone
 	roleResp, err := retrieveTestRole(codec, TestDefaultMasterToken, "dc1", existingRole.ID)
+	require.NoError(t, err)
 	require.Nil(t, roleResp.Role)
 }
 

agent/consul/acl_test.go

@@ -3041,7 +3041,7 @@ func TestACL_filterDatacenterCheckServiceNodes(t *testing.T) {
 	node_prefix "" { policy = "read" }
 	`, acl.SyntaxCurrent, nil, nil)
 	require.NoError(t, err)
-	perms, err = acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
+	_, err = acl.NewPolicyAuthorizerWithDefaults(acl.DenyAll(), []*acl.Policy{policy}, nil)
 	require.NoError(t, err)
 
 	// Now it should go through.

agent/consul/fsm/snapshot_oss_test.go

@@ -533,6 +533,7 @@ func TestFSM_SnapshotRestore_OSS(t *testing.T) {
 
 	// Verify the acl-token-bootstrap index was restored
 	canBootstrap, index, err := fsm2.state.CanBootstrapACLToken()
+	require.NoError(t, err)
 	require.False(t, canBootstrap)
 	require.True(t, index > 0)
 

agent/consul/server_lookup_test.go

@@ -1,11 +1,11 @@
 package consul
 
 import (
-	"fmt"
 	"testing"
 
 	"github.com/hashicorp/consul/agent/metadata"
 	"github.com/hashicorp/raft"
+	"github.com/stretchr/testify/require"
 )
 
 type testAddr struct {
@@ -46,11 +46,8 @@ func TestServerLookup(t *testing.T) {
 
 	lookup.RemoveServer(svr)
 
-	got, err = lookup.ServerAddr("1")
-	expectedErr := fmt.Errorf("Could not find address for server id 1")
-	if expectedErr.Error() != err.Error() {
-		t.Fatalf("Unexpected error, got %v wanted %v", err, expectedErr)
-	}
+	_, err = lookup.ServerAddr("1")
+	require.EqualError(t, err, "Could not find address for server id 1")
 
 	svr2 := &metadata.Server{ID: "2", Addr: &testAddr{"123.4.5.6"}}
 	lookup.RemoveServer(svr2)

agent/consul/state/catalog_test.go

@@ -795,7 +795,7 @@ func TestStateStore_EnsureNode(t *testing.T) {
 	if err := s.EnsureNode(3, in2); err != nil {
 		t.Fatalf("err: %s", err)
 	}
-	idx, out, err = s.GetNode("node1")
+	_, out, err = s.GetNode("node1")
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -839,7 +839,8 @@ func TestStateStore_EnsureNode(t *testing.T) {
 	}
 
 	// Retrieve the node
-	idx, out, err = s.GetNode("node1")
+	_, out, err = s.GetNode("node1")
+	require.NoError(t, err)
 	if out != nil {
 		t.Fatalf("Node should not exist anymore: %q", out)
 	}
@@ -903,7 +904,8 @@ func TestStateStore_EnsureNode(t *testing.T) {
 	}
 
 	// Retrieve the node
-	idx, out, err = s.GetNode("Node1bis")
+	_, out, err = s.GetNode("Node1bis")
+	require.NoError(t, err)
 	if out == nil {
 		t.Fatalf("Node should exist, but was null")
 	}
@@ -991,7 +993,7 @@ func TestStateStore_EnsureNode(t *testing.T) {
 	if err := s.EnsureNode(15, in); err != nil {
 		t.Fatalf("[DEPRECATED] it should work, err:= %q", err)
 	}
-	idx, out, err = s.GetNode("Node1-Renamed2")
+	_, out, err = s.GetNode("Node1-Renamed2")
 	if err != nil {
 		t.Fatalf("[DEPRECATED] err: %s", err)
 	}
@@ -2022,6 +2024,7 @@ func TestStateStore_DeleteService(t *testing.T) {
 	// Delete the service.
 	ws := memdb.NewWatchSet()
 	_, _, err := s.NodeServices(ws, "node1", nil)
+	require.NoError(t, err)
 	if err := s.DeleteService(4, "node1", "service1", nil); err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -3371,7 +3374,7 @@ func TestStateStore_ConnectQueryBlocking(t *testing.T) {
 
 			// Run the query
 			ws := memdb.NewWatchSet()
-			idx, res, err := s.CheckConnectServiceNodes(ws, tt.svc, nil)
+			_, res, err := s.CheckConnectServiceNodes(ws, tt.svc, nil)
 			require.NoError(err)
 			require.Len(res, tt.wantBeforeResLen)
 			require.Len(ws, tt.wantBeforeWatchSetSize)
@@ -3390,7 +3393,7 @@ func TestStateStore_ConnectQueryBlocking(t *testing.T) {
 
 			// Re-query the same result. Should return the desired index and len
 			ws = memdb.NewWatchSet()
-			idx, res, err = s.CheckConnectServiceNodes(ws, tt.svc, nil)
+			idx, res, err := s.CheckConnectServiceNodes(ws, tt.svc, nil)
 			require.NoError(err)
 			require.Len(res, tt.wantAfterResLen)
 			require.Equal(tt.wantAfterIndex, idx)
@@ -3463,7 +3466,7 @@ func TestStateStore_CheckServiceNodes(t *testing.T) {
 		t.Fatalf("bad")
 	}
 	ws = memdb.NewWatchSet()
-	idx, results, err = s.CheckServiceNodes(ws, "service1", nil)
+	idx, _, err = s.CheckServiceNodes(ws, "service1", nil)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -3479,7 +3482,7 @@ func TestStateStore_CheckServiceNodes(t *testing.T) {
 		t.Fatalf("bad")
 	}
 	ws = memdb.NewWatchSet()
-	idx, results, err = s.CheckServiceNodes(ws, "service1", nil)
+	idx, _, err = s.CheckServiceNodes(ws, "service1", nil)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -3493,7 +3496,7 @@ func TestStateStore_CheckServiceNodes(t *testing.T) {
 		t.Fatalf("bad")
 	}
 	ws = memdb.NewWatchSet()
-	idx, results, err = s.CheckServiceNodes(ws, "service1", nil)
+	idx, _, err = s.CheckServiceNodes(ws, "service1", nil)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}

agent/consul/state/coordinate_test.go

@@ -81,7 +81,7 @@ func TestStateStore_Coordinate_Updates(t *testing.T) {
 	require.Nil(t, all)
 
 	coordinateWs = memdb.NewWatchSet()
-	idx, coords, err = s.Coordinate("node1", coordinateWs)
+	idx, _, err = s.Coordinate("node1", coordinateWs)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}

agent/consul/state/kvs_test.go

@@ -429,7 +429,7 @@ func TestStateStore_KVSList(t *testing.T) {
 
 	// Delete a key and make sure the index comes from the tombstone.
 	ws = memdb.NewWatchSet()
-	idx, _, err = s.KVSList(ws, "foo/bar/baz", nil)
+	_, _, err = s.KVSList(ws, "foo/bar/baz", nil)
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}

agent/consul/state/session_test.go

@@ -2,12 +2,13 @@ package state
 
 import (
 	"fmt"
-	"github.com/stretchr/testify/assert"
 	"reflect"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/types"
@@ -543,7 +544,7 @@ func TestStateStore_Session_Invalidate_DeleteNode(t *testing.T) {
 
 	// Delete the node and make sure the watch fires.
 	ws := memdb.NewWatchSet()
-	idx, s2, err := s.SessionGet(ws, session.ID, nil)
+	_, _, err := s.SessionGet(ws, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -555,7 +556,7 @@ func TestStateStore_Session_Invalidate_DeleteNode(t *testing.T) {
 	}
 
 	// Lookup by ID, should be nil.
-	idx, s2, err = s.SessionGet(nil, session.ID, nil)
+	idx, s2, err := s.SessionGet(nil, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -598,7 +599,7 @@ func TestStateStore_Session_Invalidate_DeleteService(t *testing.T) {
 
 	// Delete the service and make sure the watch fires.
 	ws := memdb.NewWatchSet()
-	idx, s2, err := s.SessionGet(ws, session.ID, nil)
+	_, _, err := s.SessionGet(ws, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -610,7 +611,7 @@ func TestStateStore_Session_Invalidate_DeleteService(t *testing.T) {
 	}
 
 	// Lookup by ID, should be nil.
-	idx, s2, err = s.SessionGet(nil, session.ID, nil)
+	idx, s2, err := s.SessionGet(nil, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -648,7 +649,7 @@ func TestStateStore_Session_Invalidate_Critical_Check(t *testing.T) {
 
 	// Invalidate the check and make sure the watches fire.
 	ws := memdb.NewWatchSet()
-	idx, s2, err := s.SessionGet(ws, session.ID, nil)
+	_, _, err := s.SessionGet(ws, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -661,7 +662,7 @@ func TestStateStore_Session_Invalidate_Critical_Check(t *testing.T) {
 	}
 
 	// Lookup by ID, should be nil.
-	idx, s2, err = s.SessionGet(nil, session.ID, nil)
+	idx, s2, err := s.SessionGet(nil, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -699,7 +700,7 @@ func TestStateStore_Session_Invalidate_DeleteCheck(t *testing.T) {
 
 	// Delete the check and make sure the watches fire.
 	ws := memdb.NewWatchSet()
-	idx, s2, err := s.SessionGet(ws, session.ID, nil)
+	_, _, err := s.SessionGet(ws, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -711,7 +712,7 @@ func TestStateStore_Session_Invalidate_DeleteCheck(t *testing.T) {
 	}
 
 	// Lookup by ID, should be nil.
-	idx, s2, err = s.SessionGet(nil, session.ID, nil)
+	idx, s2, err := s.SessionGet(nil, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -767,7 +768,7 @@ func TestStateStore_Session_Invalidate_Key_Unlock_Behavior(t *testing.T) {
 
 	// Delete the node and make sure the watches fire.
 	ws := memdb.NewWatchSet()
-	idx, s2, err := s.SessionGet(ws, session.ID, nil)
+	_, _, err = s.SessionGet(ws, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -779,7 +780,7 @@ func TestStateStore_Session_Invalidate_Key_Unlock_Behavior(t *testing.T) {
 	}
 
 	// Lookup by ID, should be nil.
-	idx, s2, err = s.SessionGet(nil, session.ID, nil)
+	idx, s2, err := s.SessionGet(nil, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -849,7 +850,7 @@ func TestStateStore_Session_Invalidate_Key_Delete_Behavior(t *testing.T) {
 
 	// Delete the node and make sure the watches fire.
 	ws := memdb.NewWatchSet()
-	idx, s2, err := s.SessionGet(ws, session.ID, nil)
+	_, _, err = s.SessionGet(ws, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -861,7 +862,7 @@ func TestStateStore_Session_Invalidate_Key_Delete_Behavior(t *testing.T) {
 	}
 
 	// Lookup by ID, should be nil.
-	idx, s2, err = s.SessionGet(nil, session.ID, nil)
+	idx, s2, err := s.SessionGet(nil, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -917,7 +918,7 @@ func TestStateStore_Session_Invalidate_PreparedQuery_Delete(t *testing.T) {
 
 	// Invalidate the session and make sure the watches fire.
 	ws := memdb.NewWatchSet()
-	idx, s2, err := s.SessionGet(ws, session.ID, nil)
+	_, _, err := s.SessionGet(ws, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -929,7 +930,7 @@ func TestStateStore_Session_Invalidate_PreparedQuery_Delete(t *testing.T) {
 	}
 
 	// Make sure the session is gone.
-	idx, s2, err = s.SessionGet(nil, session.ID, nil)
+	idx, s2, err := s.SessionGet(nil, session.ID, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}