Merge pull request #311 from cashapp/sahilm/upgrade-to-latest-golangc…

…i-lint

[#310] upgrade to latest golangci-lint and enable new linters

.github/workflows/linter.yml

@@ -22,8 +22,8 @@ jobs:
       - name: Checkout code
         uses: actions/checkout@v4
       - name: golangci-lint
-        uses: golangci/golangci-lint-action@v3
+        uses: golangci/golangci-lint-action@v6
         with:
           # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version
-          version: v1.54.2
+          version: v1.59.1
           args: --timeout=2m

.golangci.yaml

@@ -9,6 +9,7 @@ linters:
   disable:
     - tagalign
     - depguard
+    - mnd
     - musttag
     - nonamedreturns
     - exhaustruct
@@ -26,7 +27,6 @@ linters:
     - ireturn
     - maintidx
     - nilnil
-    - maligned
     - lll
     - gochecknoglobals
     - wsl
@@ -36,21 +36,16 @@ linters:
     - goprintffuncname
     - paralleltest
     - nlreturn
-    - goerr113
-    - ifshort
+    - err113
     - testpackage
     - wrapcheck
-    - exhaustivestruct
     - forbidigo
     - gci
     - godot
     - gofumpt
     - cyclop
     - errorlint
     - nestif
-    - golint
-    - scopelint
-    - interfacer
     - tagliatelle
     - thelper
     - godox
@@ -65,6 +60,12 @@ linters-settings:
     min-complexity: 20
   exhaustive:
     default-signifies-exhaustive: true
+  testifylint:
+    enable-all: true
+    disable:
+      - suite-thelper # this is disabled by default
+      - float-compare
+      - require-error
 
 issues:
   max-per-linter: 0
@@ -79,4 +80,3 @@ issues:
     - 'bad syntax for struct tag key'
     - 'bad syntax for struct tag pair'
     - 'result .* \(error\) is always nil'
-

pkg/check/illegalclause.go

@@ -2,7 +2,6 @@ package check
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/cashapp/spirit/pkg/utils"
 	"github.com/siddontang/loggers"
@@ -15,6 +14,6 @@ func init() {
 // illegalClauseCheck checks for the presence of specific, unsupported
 // clauses in the ALTER statement, such as ALGORITHM= and LOCK=.
 func illegalClauseCheck(ctx context.Context, r Resources, logger loggers.Advanced) error {
-	sql := fmt.Sprintf("ALTER TABLE x.x %s", r.Alter)
+	sql := "ALTER TABLE x.x " + r.Alter
 	return utils.AlterContainsUnsupportedClause(sql)
 }

pkg/checksum/checker.go

@@ -90,7 +90,7 @@ func NewChecker(db *sql.DB, tbl, newTable *table.TableInfo, feed *repl.Client, c
 	return checksum, nil
 }
 
-func (c *Checker) ChecksumChunk(trxPool *dbconn.TrxPool, chunk *table.Chunk) error {
+func (c *Checker) ChecksumChunk(ctx context.Context, trxPool *dbconn.TrxPool, chunk *table.Chunk) error {
 	startTime := time.Now()
 	trx, err := trxPool.Get()
 	if err != nil {
@@ -131,7 +131,7 @@ func (c *Checker) ChecksumChunk(trxPool *dbconn.TrxPool, chunk *table.Chunk) err
 			return errors.New("checksum mismatch")
 		}
 		// Since we can fix differences, replace the chunk.
-		if err = c.replaceChunk(chunk); err != nil {
+		if err = c.replaceChunk(ctx, chunk); err != nil {
 			return err
 		}
 	}
@@ -220,7 +220,7 @@ SELECT source.row_checksum as source_row_checksum, target.row_checksum as target
 // that it takes to copy the data. Maybe in future we could consider splitting
 // the chunk here, but this is expected to be a very rare situation, so a small
 // stall from an XL sized chunk is considered acceptable.
-func (c *Checker) replaceChunk(chunk *table.Chunk) error {
+func (c *Checker) replaceChunk(ctx context.Context, chunk *table.Chunk) error {
 	c.logger.Warnf("recopying chunk: %s", chunk.String())
 	deleteStmt := "DELETE FROM " + c.newTable.QuotedName + " WHERE " + chunk.String()
 	replaceStmt := fmt.Sprintf("REPLACE INTO %s (%s) SELECT %s FROM %s WHERE %s",
@@ -292,10 +292,10 @@ func (c *Checker) replaceChunk(chunk *table.Chunk) error {
 	c.recopyLock.Lock()
 	defer c.recopyLock.Unlock()
 
-	if _, err := dbconn.RetryableTransaction(context.TODO(), c.db, false, dbconn.NewDBConfig(), deleteStmt); err != nil {
+	if _, err := dbconn.RetryableTransaction(ctx, c.db, false, dbconn.NewDBConfig(), deleteStmt); err != nil {
 		return err
 	}
-	if _, err := dbconn.RetryableTransaction(context.TODO(), c.db, false, dbconn.NewDBConfig(), replaceStmt); err != nil {
+	if _, err := dbconn.RetryableTransaction(ctx, c.db, false, dbconn.NewDBConfig(), replaceStmt); err != nil {
 		return err
 	}
 	return nil
@@ -396,7 +396,7 @@ func (c *Checker) Run(ctx context.Context) error {
 				c.setInvalid(true)
 				return err
 			}
-			if err := c.ChecksumChunk(c.trxPool, chunk); err != nil {
+			if err := c.ChecksumChunk(errGrpCtx, c.trxPool, chunk); err != nil {
 				c.setInvalid(true)
 				return err
 			}

pkg/dbconn/conn.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"net/url"
 	"regexp"
+	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -3023,9 +3024,9 @@ func newDSN(dsn string, config *DBConfig) (string, error) {
 	// they all unset the SQL mode just like this.
 	ops = append(ops, fmt.Sprintf("%s=%s", "sql_mode", url.QueryEscape(`""`)))
 	ops = append(ops, fmt.Sprintf("%s=%s", "time_zone", url.QueryEscape(`"+00:00"`)))
-	ops = append(ops, fmt.Sprintf("%s=%s", "innodb_lock_wait_timeout", url.QueryEscape(fmt.Sprint(config.InnodbLockWaitTimeout))))
-	ops = append(ops, fmt.Sprintf("%s=%s", "lock_wait_timeout", url.QueryEscape(fmt.Sprint(config.LockWaitTimeout))))
-	ops = append(ops, fmt.Sprintf("%s=%s", "range_optimizer_max_mem_size", url.QueryEscape(fmt.Sprint(config.RangeOptimizerMaxMemSize))))
+	ops = append(ops, fmt.Sprintf("%s=%s", "innodb_lock_wait_timeout", url.QueryEscape(strconv.Itoa(config.InnodbLockWaitTimeout))))
+	ops = append(ops, fmt.Sprintf("%s=%s", "lock_wait_timeout", url.QueryEscape(strconv.Itoa(config.LockWaitTimeout))))
+	ops = append(ops, fmt.Sprintf("%s=%s", "range_optimizer_max_mem_size", url.QueryEscape(strconv.FormatInt(config.RangeOptimizerMaxMemSize, 10))))
 	ops = append(ops, fmt.Sprintf("%s=%s", "transaction_isolation", url.QueryEscape(`"read-committed"`)))
 	// go driver options, should set:
 	// character_set_client, character_set_connection, character_set_results

pkg/dbconn/dbconn.go

@@ -4,6 +4,7 @@ package dbconn
 import (
 	"context"
 	"database/sql"
+	"errors"
 	"fmt"
 	"math/rand"
 	"time"
@@ -69,7 +70,7 @@ func RetryableTransaction(ctx context.Context, db *sql.DB, ignoreDupKeyWarnings
 		rowsAffected int64
 		isFatal      bool
 	)
-	for i := 0; i < config.MaxRetries; i++ {
+	for i := range config.MaxRetries {
 		func() {
 			// Start a transaction
 			if trx, err = db.BeginTx(ctx, nil); err != nil {
@@ -127,7 +128,7 @@ func RetryableTransaction(ctx context.Context, db *sql.DB, ignoreDupKeyWarnings
 						// to be ignored. *However* if range optimization is disabled the query is going to
 						// tablescan, so it's better to just bail out and present a useful error message.
 						isFatal = true
-						err = fmt.Errorf("MySQL refused to optimize a statement because the value of 'range_optimizer_max_mem_size' is too low. Please decrease the target-chunk-time, or increase the value of 'range_optimizer_max_mem_size'")
+						err = errors.New("MySQL refused to optimize a statement because the value of 'range_optimizer_max_mem_size' is too low. Please decrease the target-chunk-time, or increase the value of 'range_optimizer_max_mem_size'")
 						return
 					} else {
 						isFatal = true

pkg/dbconn/dbconn_test.go

@@ -3,7 +3,7 @@ package dbconn
 import (
 	"context"
 	"database/sql"
-	"fmt"
+	"strconv"
 	"testing"
 	"time"
 
@@ -33,11 +33,11 @@ func TestLockWaitTimeouts(t *testing.T) {
 
 	lockWaitTimeout, err := getVariable(trx, "lock_wait_timeout", true)
 	assert.NoError(t, err)
-	assert.Equal(t, fmt.Sprint(config.LockWaitTimeout), lockWaitTimeout)
+	assert.Equal(t, strconv.Itoa(config.LockWaitTimeout), lockWaitTimeout)
 
 	innodbLockWaitTimeout, err := getVariable(trx, "innodb_lock_wait_timeout", true)
 	assert.NoError(t, err)
-	assert.Equal(t, fmt.Sprint(config.InnodbLockWaitTimeout), innodbLockWaitTimeout)
+	assert.Equal(t, strconv.Itoa(config.InnodbLockWaitTimeout), innodbLockWaitTimeout)
 }
 
 func TestRetryableTrx(t *testing.T) {

pkg/dbconn/sqlescape/utils_test.go

@@ -25,7 +25,7 @@ import (
 
 func TestReserveBuffer(t *testing.T) {
 	res0 := reserveBuffer(nil, 0)
-	require.Len(t, res0, 0)
+	require.Empty(t, res0)
 
 	res1 := reserveBuffer(res0, 3)
 	require.Len(t, res1, 3)
@@ -96,8 +96,6 @@ func TestEscapeBackslash(t *testing.T) {
 		},
 	}
 	for _, v := range tests {
-		// copy iterator variable into a new variable, see issue #27779
-		v := v
 		t.Run(v.name, func(t *testing.T) {
 			require.Equal(t, v.output, escapeBytesBackslash(nil, v.input))
 			require.Equal(t, v.output, escapeStringBackslash(nil, string(v.input)))
@@ -402,8 +400,6 @@ func TestEscapeSQL(t *testing.T) {
 		},
 	}
 	for _, v := range tests {
-		// copy iterator variable into a new variable, see issue #27779
-		v := v
 		t.Run(v.name, func(t *testing.T) {
 			r3 := new(strings.Builder)
 			r1, e1 := escapeSQL(v.input, v.params...)

pkg/dbconn/tablelock.go

@@ -33,7 +33,7 @@ func NewTableLock(ctx context.Context, db *sql.DB, table *table.TableInfo, write
 	var err error
 	var isFatal bool
 	var lockTxn *sql.Tx
-	for i := 0; i < config.MaxRetries; i++ {
+	for i := range config.MaxRetries {
 		func() {
 			lockTxn, _ = db.BeginTx(ctx, nil)
 			defer func() {

pkg/dbconn/trxpool.go

@@ -22,7 +22,7 @@ type TrxPool struct {
 // had their read-view created in REPEATABLE READ isolation.
 func NewTrxPool(ctx context.Context, db *sql.DB, count int, config *DBConfig) (*TrxPool, error) {
 	checksumTxns := make([]*sql.Tx, 0, count)
-	for i := 0; i < count; i++ {
+	for range count {
 		trx, _ := db.BeginTx(ctx, &sql.TxOptions{Isolation: sql.LevelRepeatableRead})
 		_, err := trx.ExecContext(ctx, "START TRANSACTION WITH CONSISTENT SNAPSHOT")
 		if err != nil {

pkg/metrics/metrics.go

@@ -41,7 +41,7 @@ type MetricValue struct {
 // Sink sends metrics to an external destination.
 type Sink interface {
 	// Send sends metrics to the sink. It must respect the context timeout, if any.
-	Send(context.Context, *Metrics) error
+	Send(ctx context.Context, metrics *Metrics) error
 }
 
 // NoopSink is the default sink which does nothing

pkg/migration/cutover.go

@@ -51,7 +51,7 @@ func (c *CutOver) Run(ctx context.Context) error {
 		// Because we want to safely flush quickly, we set the limit to 5.
 		c.db.SetMaxOpenConns(5)
 	}
-	for i := 0; i < c.dbConfig.MaxRetries; i++ {
+	for i := range c.dbConfig.MaxRetries {
 		if ctx.Err() != nil {
 			return ctx.Err()
 		}

pkg/migration/runner.go

@@ -804,7 +804,7 @@ func (r *Runner) checksum(ctx context.Context) error {
 	}
 	r.db.SetMaxOpenConns(r.dbConfig.MaxOpenConnections + 2)
 	var err error
-	for i := 0; i < 3; i++ { // try the checksum up to 3 times.
+	for i := range 3 { // try the checksum up to 3 times.
 		r.checkerLock.Lock()
 		r.checker, err = checksum.NewChecker(r.db, r.table, r.newTable, r.replClient, &checksum.CheckerConfig{
 			Concurrency:     r.migration.Threads,
@@ -835,9 +835,9 @@ func (r *Runner) checksum(ctx context.Context) error {
 			// do our best-case to differentiate that we believe this ALTER statement is lossy, and
 			// customize the returned error based on it.
 			if err := utils.AlterContainsAddUnique("ALTER TABLE unused " + r.migration.Alter); err != nil {
-				return fmt.Errorf("checksum failed after 3 attempts. Check that the ALTER statement is not adding a UNIQUE INDEX to non-unique data")
+				return errors.New("checksum failed after 3 attempts. Check that the ALTER statement is not adding a UNIQUE INDEX to non-unique data")
 			}
-			return fmt.Errorf("checksum failed after 3 attempts. This likely indicates either a bug in Spirit, or a manual modification to the _new table outside of Spirit. Please report @ github.com/cashapp/spirit")
+			return errors.New("checksum failed after 3 attempts. This likely indicates either a bug in Spirit, or a manual modification to the _new table outside of Spirit. Please report @ github.com/cashapp/spirit")
 		}
 		r.logger.Errorf("checksum failed, retrying %d/%d times", i+1, 3)
 	}