[ci] Add errcheck to golangci-lint

.github/workflows/golangci-linter.yml

@@ -25,7 +25,7 @@ jobs:
       uses: actions/checkout@v2
 
     - name: Install golangci-lint
-      run: curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b $(go env GOPATH)/bin v1.31.0
+      run: curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b $(go env GOPATH)/bin v1.39.0
 
     - name: Clean Env
       run: $(go env GOPATH)/bin/golangci-lint cache clean

.golangci.yml

@@ -4,6 +4,8 @@ run:
     - go/vt/topo/k8stopo/client
 
 linters-settings:
+  errcheck:
+    exclude: ./misc/errcheck_excludes.txt
   goimports:
     local-prefixes: vitess.io/vitess
 
@@ -12,7 +14,7 @@ linters:
   enable:
     # Defaults
     - deadcode
-    # - errcheck
+    - errcheck
     - gosimple
     - govet
     - ineffassign
@@ -29,8 +31,113 @@ issues:
   exclude-rules:
     - path: '^go/vt/proto/'
       linters:
+      - errcheck
       - goimports
 
+    ### BEGIN: errcheck exclusion rules. Each rule should be considered
+    #   a TODO for removal after adding error checks to that package/file/etc,
+    #   except where otherwise noted.
+    - path: '^go/cmd/(vtcombo|vtgateclienttest|vtorc)/'
+      linters:
+      - errcheck
+    - path: '^go/mysql/'
+      linters:
+      - errcheck
+    - path: '^go/pools/.*_test.go'
+      linters:
+      - errcheck
+    - path: '^go/sqltypes/'
+      linters:
+      - errcheck
+    - path: '^go/stats/statsd/'
+      linters:
+      - errcheck
+    - path: '^go/test/'
+      linters:
+      - errcheck
+    - path: '^go/vt/automation/'
+      linters:
+      - errcheck
+    - path: '^go/vt/mysqlctl/'
+      linters:
+      - errcheck
+    # (NB: @ajm188) Technically, the below `exclude-rules` for `go/vt/orchestrator/external` is
+    # redundant with this line, but we actually want to permanently ignore orchestrator's
+    # vendored code, so we are keeping the exclusion rules separate because this one
+    # should be temporary while the latter should be permanent.
+    - path: '^go/vt/orchestrator/'
+      linters:
+      - errcheck
+    # This subtree should be permanently excluded, as it's vendored code.
+    - path: '^go/vt/orchestrator/external/'
+      linters:
+      - errcheck
+    - path: '^go/vt/schemamanager/'
+      linters:
+      - errcheck
+    - path: '^go/vt/servenv/'
+      linters:
+      - errcheck
+    # This code is autogenerated and should be permanently excluded.
+    - path: '^go/vt/sqlparser/goyacc'
+      linters:
+      - errcheck
+    - path: '^go/vt/throttler/.*_test.go'
+      linters:
+      - errcheck
+    - path: '^go/vt/topo/.*/*._test.go'
+      linters:
+      - errcheck
+    - path: '^go/vt/vtcombo/'
+      linters:
+      - errcheck
+    - path: '^go/vt/vtctl/[^/]*.go'
+      linters:
+      - errcheck
+    - path: '^go/vt/vtctl/grpcvtctlclient/'
+      linters:
+      - errcheck
+    - path: '^go/vt/vtctl/grpcvtctlserver/'
+      linters:
+      - errcheck
+    - path: '^go/vt/vtctld/(schema|.*_test).go'
+      linters:
+      - errcheck
+    - path: '^go/vt/vtexplain/'
+      linters:
+      - errcheck
+    - path: '^go/vt/vtgate/.*_test.go'
+      linters:
+      - errcheck
+    - path: '^go/vt/vtgr/'
+      linters:
+      - errcheck
+    - path: '^go/vt/vttablet/(customrule|filelogger|grpctmserver|onlineddl|sandboxconn|tabletserver)/'
+      linters:
+      - errcheck
+    - path: '^go/vt/vttablet/tabletmanager/vreplication'
+      linters:
+      - errcheck
+    - path: '^go/vt/vttablet/(.*endtoend.*|.*_test.go)'
+      linters:
+      - errcheck
+    - path: '^go/vt/vttest'
+      linters:
+      - errcheck
+    - path: '^go/vt/worker'
+      linters:
+      - errcheck
+    - path: '^go/vt/workflow'
+      linters:
+      - errcheck
+    - path: '^go/vt/wrangler'
+      linters:
+      - errcheck
+    - path: '^go/vt/zkctl'
+      linters:
+      - errcheck
+    ### END: errcheck exclusion rules
+
 # https://github.com/golangci/golangci/wiki/Configuration
 service:
-  golangci-lint-version: 1.31.0 # use the fixed version to not introduce new linters unexpectedly
+  golangci-lint-version: 1.39.0 # use the fixed version to not introduce new linters unexpectedly

go/cache/ristretto/bloom/bbloom_test.go

@@ -19,7 +19,7 @@ func TestMain(m *testing.M) {
 	wordlist1 = make([][]byte, n)
 	for i := range wordlist1 {
 		b := make([]byte, 32)
-		rand.Read(b)
+		_, _ = rand.Read(b)
 		wordlist1[i] = b
 	}
 	fmt.Println("\n###############\nbbloom_test.go")

go/cmd/mysqlctld/mysqlctld.go

@@ -124,7 +124,9 @@ func main() {
 	servenv.OnTermSync(func() {
 		log.Infof("mysqlctl received SIGTERM, shutting down mysqld first")
 		ctx := context.Background()
-		mysqld.Shutdown(ctx, cnf, true)
+		if err := mysqld.Shutdown(ctx, cnf, true); err != nil {
+			log.Errorf("failed to shutdown mysqld: %v", err)
+		}
 	})
 
 	// Start RPC server and wait for SIGTERM.

go/cmd/rulesctl/cmd/list.go

@@ -40,11 +40,13 @@ func List() *cobra.Command {
 				out = rules
 			}
 		} else {
-			out = rules.Find(listOptName)
-			if listOptNamesOnly && out != nil {
+			rule := rules.Find(listOptName)
+			if listOptNamesOnly && rule != nil {
 				out = listOptName
 			} else if listOptNamesOnly {
 				out = ""
+			} else {
+				out = rule
 			}
 		}
 

go/cmd/vtbackup/vtbackup.go

@@ -235,7 +235,9 @@ func takeBackup(ctx context.Context, topoServer *topo.Server, backupStorage back
 		// skip shutdown just because we timed out waiting for other things.
 		ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 		defer cancel()
-		mysqld.Shutdown(ctx, mycnf, false)
+		if err := mysqld.Shutdown(ctx, mycnf, false); err != nil {
+			log.Errorf("failed to shutdown mysqld: %v", err)
+		}
 	}()
 
 	extraEnv := map[string]string{

go/cmd/vtclient/vtclient_test.go

@@ -77,7 +77,7 @@ func TestVtclient(t *testing.T) {
 	if err := cluster.Setup(); err != nil {
 		t.Fatalf("InitSchemas failed: %v", err)
 	}
-	defer cluster.TearDown()
+	defer cluster.TearDown() // nolint:errcheck
 
 	vtgateAddr := fmt.Sprintf("localhost:%v", cluster.Env.PortForProtocol("vtcombo", "grpc"))
 	queries := []struct {

go/cmd/vtctl/vtctl.go

@@ -85,7 +85,7 @@ func main() {
 	startMsg := fmt.Sprintf("USER=%v SUDO_USER=%v %v", os.Getenv("USER"), os.Getenv("SUDO_USER"), strings.Join(os.Args, " "))
 
 	if syslogger, err := syslog.New(syslog.LOG_INFO, "vtctl "); err == nil {
-		syslogger.Info(startMsg)
+		syslogger.Info(startMsg) // nolint:errcheck
 	} else {
 		log.Warningf("cannot connect to syslog: %v", err)
 	}

go/cmd/vttestserver/vttestserver_test.go

@@ -135,7 +135,7 @@ func TestForeignKeysAndDDLModes(t *testing.T) {
 	assert.NoError(t, err)
 	defer cluster.TearDown()
 
-	execOnCluster(cluster, "test_keyspace", func(conn *mysql.Conn) error {
+	err = execOnCluster(cluster, "test_keyspace", func(conn *mysql.Conn) error {
 		_, err := conn.ExecuteFetch(`CREATE TABLE test_table_2 (
 			id BIGINT,
 			test_table_id BIGINT,
@@ -154,13 +154,14 @@ func TestForeignKeysAndDDLModes(t *testing.T) {
 		assert.NoError(t, err)
 		return nil
 	})
+	assert.NoError(t, err)
 
 	cluster.TearDown()
 	cluster, err = startCluster("-foreign_key_mode=disallow", "-enable_online_ddl=false", "-enable_direct_ddl=false")
 	assert.NoError(t, err)
 	defer cluster.TearDown()
 
-	execOnCluster(cluster, "test_keyspace", func(conn *mysql.Conn) error {
+	err = execOnCluster(cluster, "test_keyspace", func(conn *mysql.Conn) error {
 		_, err := conn.ExecuteFetch(`CREATE TABLE test_table_2 (
 			id BIGINT,
 			test_table_id BIGINT,
@@ -177,6 +178,7 @@ func TestForeignKeysAndDDLModes(t *testing.T) {
 		assert.Error(t, err)
 		return nil
 	})
+	assert.NoError(t, err)
 }
 
 func TestCanVtGateExecute(t *testing.T) {

go/streamlog/streamlog.go

@@ -179,7 +179,7 @@ func (logger *StreamLogger) LogToFile(path string, logf LogFormatter) (chan inte
 		for {
 			select {
 			case record := <-logChan:
-				logf(f, formatParams, record)
+				logf(f, formatParams, record) // nolint:errcheck
 			case <-rotateChan:
 				f.Close()
 				f, _ = os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0644)

go/streamlog/streamlog_flaky_test.go

@@ -235,7 +235,9 @@ func TestFile(t *testing.T) {
 
 	// Send the rotate signal which should reopen the original file path
 	// for new logs to go to
-	syscall.Kill(syscall.Getpid(), syscall.SIGUSR2)
+	if err := syscall.Kill(syscall.Getpid(), syscall.SIGUSR2); err != nil {
+		t.Logf("failed to send streamlog rotate signal: %v", err)
+	}
 	time.Sleep(10 * time.Millisecond)
 
 	logger.Send(&logMessage{"test 4"})

go/vt/binlog/binlogplayer/binlog_player.go

@@ -741,7 +741,9 @@ func MysqlUncompress(input string) []byte {
 		return nil
 	}
 	var outputBytes bytes.Buffer
-	io.Copy(&outputBytes, reader)
+	if _, err := io.Copy(&outputBytes, reader); err != nil {
+		return nil
+	}
 	if outputBytes.Len() == 0 {
 		return nil
 	}

go/vt/binlog/binlogplayertest/player.go

@@ -112,7 +112,9 @@ func (fake *FakeBinlogStreamer) StreamKeyRange(ctx context.Context, position str
 		!proto.Equal(charset, testKeyRangeRequest.Charset) {
 		fake.t.Errorf("wrong StreamKeyRange parameter, got %+v want %+v", req, testKeyRangeRequest)
 	}
-	callback(testBinlogTransaction)
+	if err := callback(testBinlogTransaction); err != nil {
+		fake.t.Logf("StreamKeyRange callback failed: %v", err)
+	}
 	return nil
 }
 
@@ -178,7 +180,9 @@ func (fake *FakeBinlogStreamer) StreamTables(ctx context.Context, position strin
 		!proto.Equal(charset, testTablesRequest.Charset) {
 		fake.t.Errorf("wrong StreamTables parameter, got %+v want %+v", req, testTablesRequest)
 	}
-	callback(testBinlogTransaction)
+	if err := callback(testBinlogTransaction); err != nil {
+		fake.t.Logf("StreamTables callback failed: %v", err)
+	}
 	return nil
 }
 

go/vt/binlog/tables_filter_test.go

@@ -58,7 +58,7 @@ func TestTablesFilterPass(t *testing.T) {
 		got = bltToString(reply)
 		return nil
 	})
-	f(eventToken, statements)
+	_ = f(eventToken, statements)
 	want := `statement: <6, "set1"> statement: <7, "dml1 /* _stream included1 (id ) (500 ); */"> statement: <7, "dml2 /* _stream included2 (id ) (500 ); */"> position: "MariaDB/0-41983-1" `
 	if want != got {
 		t.Errorf("want\n%s, got\n%s", want, got)
@@ -88,7 +88,7 @@ func TestTablesFilterSkip(t *testing.T) {
 		got = bltToString(reply)
 		return nil
 	})
-	f(eventToken, statements)
+	_ = f(eventToken, statements)
 	want := `position: "MariaDB/0-41983-1" `
 	if want != got {
 		t.Errorf("want %s, got %s", want, got)
@@ -118,7 +118,7 @@ func TestTablesFilterDDL(t *testing.T) {
 		got = bltToString(reply)
 		return nil
 	})
-	f(eventToken, statements)
+	_ = f(eventToken, statements)
 	want := `position: "MariaDB/0-41983-1" `
 	if want != got {
 		t.Errorf("want %s, got %s", want, got)
@@ -154,7 +154,7 @@ func TestTablesFilterMalformed(t *testing.T) {
 		got = bltToString(reply)
 		return nil
 	})
-	f(eventToken, statements)
+	_ = f(eventToken, statements)
 	want := `position: "MariaDB/0-41983-1" `
 	if want != got {
 		t.Errorf("want %s, got %s", want, got)

go/vt/dbconfigs/dbconfigs_test.go

@@ -283,7 +283,7 @@ func hupTest(t *testing.T, tmpFile *os.File, oldStr, newStr string) {
 	if pass != oldStr {
 		t.Fatalf("%s's Password should still be '%s'", oldStr, oldStr)
 	}
-	syscall.Kill(syscall.Getpid(), syscall.SIGHUP)
+	_ = syscall.Kill(syscall.Getpid(), syscall.SIGHUP)
 	time.Sleep(100 * time.Millisecond) // wait for signal handler
 	_, _, err := cs.GetUserAndPassword(oldStr)
 	if err != ErrUnknownUser {

go/vt/discovery/healthcheck_test.go

@@ -1062,7 +1062,7 @@ func TestCellAliases(t *testing.T) {
 		Cells: []string{"cell1", "cell2"},
 	}
 	assert.Nil(t, ts.CreateCellsAlias(context.Background(), "region1", cellsAlias), "failed to create cell alias")
-	defer ts.DeleteCellsAlias(context.Background(), "region1")
+	defer deleteCellsAlias(t, ts, "region1")
 
 	// add a tablet as replica in diff cell, same region
 	tablet := createTestTablet(1, "cell2", "host2")
@@ -1311,3 +1311,9 @@ func createTestTablet(uid uint32, cell, host string) *topodatapb.Tablet {
 }
 
 var mustMatch = utils.MustMatchFn(".Conn" /* ignored fields*/)
+
+func deleteCellsAlias(t *testing.T, ts *topo.Server, alias string) {
+	if err := ts.DeleteCellsAlias(context.Background(), alias); err != nil {
+		t.Logf("DeleteCellsAlias(%s) failed: %v", alias, err)
+	}
+}

go/vt/discovery/legacy_tablet_stats_cache_test.go

@@ -41,7 +41,7 @@ func TestLegacyTabletStatsCache(t *testing.T) {
 		log.Errorf("creating cellsAlias \"region1\" failed: %v", err)
 	}
 
-	defer ts.DeleteCellsAlias(context.Background(), "region1")
+	defer deleteCellsAlias(t, ts, "region1")
 
 	cellsAlias = &topodatapb.CellsAlias{
 		Cells: []string{"cell2"},
@@ -51,7 +51,7 @@ func TestLegacyTabletStatsCache(t *testing.T) {
 		log.Errorf("creating cellsAlias \"region2\" failed: %v", err)
 	}
 
-	defer ts.DeleteCellsAlias(context.Background(), "region2")
+	defer deleteCellsAlias(t, ts, "region2")
 
 	// We want to unit test LegacyTabletStatsCache without a full-blown
 	// LegacyHealthCheck object, so we can't call NewLegacyTabletStatsCache.

go/vt/discovery/tablet_health.go

@@ -89,6 +89,6 @@ func (th *TabletHealth) GetHostNameLevel(level int) string {
 // https://{{.GetHostNameLevel 0}}.bastion.corp -> https://host.bastion.corp
 func (th *TabletHealth) getTabletDebugURL() string {
 	var buffer bytes.Buffer
-	tabletURLTemplate.Execute(&buffer, th)
+	_ = tabletURLTemplate.Execute(&buffer, th)
 	return buffer.String()
 }

go/vt/key/key_test.go

@@ -697,7 +697,9 @@ func BenchmarkKeyRangesOverlap(b *testing.B) {
 	}
 
 	for i := 0; i < b.N; i++ {
-		KeyRangesOverlap(kr1, kr2)
+		if _, err := KeyRangesOverlap(kr1, kr2); err != nil {
+			b.Fatal(err)
+		}
 	}
 }