Add golangci-lint CI action, fix gosimple, govet + unused lint errors (#1127)

* Add `golangci-lint`, fix `gosimple`, `govet` and `unused` linter complaints

* Go 1.16

* Update copyright dates

Author: timvaillancourt

.github/workflows/golangci-lint.yml

@@ -0,0 +1,21 @@
+name: golangci-lint
+on:
+  push:
+    branches:
+      - master
+  pull_request:
+permissions:
+  contents: read
+  # Optional: allow read access to pull request. Use with `only-new-issues` option.
+  # pull-requests: read
+jobs:
+  golangci:
+    name: lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/setup-go@v3
+        with:
+          go-version: 1.16
+      - uses: actions/checkout@v3
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@v3

.golangci.yml

@@ -0,0 +1,12 @@
+run:
+  timeout: 5m
+  modules-download-mode: readonly
+
+linters:
+  disable:
+    - errcheck
+    - staticcheck
+  enable:
+    - gosimple
+    - govet
+    - unused

go/base/context.go

@@ -1,5 +1,5 @@
 /*
-   Copyright 2016 GitHub Inc.
+   Copyright 2022 GitHub Inc.
 	 See https://github.com/github/gh-ost/blob/master/LICENSE
 */
 
@@ -15,7 +15,7 @@ import (
 	"sync/atomic"
 	"time"
 
-	"github.com/satori/go.uuid"
+	uuid "github.com/satori/go.uuid"
 
 	"github.com/github/gh-ost/go/mysql"
 	"github.com/github/gh-ost/go/sql"

go/base/context_test.go

@@ -1,11 +1,13 @@
 /*
-   Copyright 2016 GitHub Inc.
+   Copyright 2022 GitHub Inc.
 	 See https://github.com/github/gh-ost/blob/master/LICENSE
 */
 
 package base
 
 import (
+	"io/ioutil"
+	"os"
 	"testing"
 	"time"
 
@@ -56,3 +58,65 @@ func TestGetTableNames(t *testing.T) {
 		test.S(t).ExpectEquals(context.GetChangelogTableName(), "_tmp_ghc")
 	}
 }
+
+func TestReadConfigFile(t *testing.T) {
+	{
+		context := NewMigrationContext()
+		context.ConfigFile = "/does/not/exist"
+		if err := context.ReadConfigFile(); err == nil {
+			t.Fatal("Expected .ReadConfigFile() to return an error, got nil")
+		}
+	}
+	{
+		f, err := ioutil.TempFile("", t.Name())
+		if err != nil {
+			t.Fatalf("Failed to create tmp file: %v", err)
+		}
+		defer os.Remove(f.Name())
+
+		f.Write([]byte("[client]"))
+		context := NewMigrationContext()
+		context.ConfigFile = f.Name()
+		if err := context.ReadConfigFile(); err != nil {
+			t.Fatalf(".ReadConfigFile() failed: %v", err)
+		}
+	}
+	{
+		f, err := ioutil.TempFile("", t.Name())
+		if err != nil {
+			t.Fatalf("Failed to create tmp file: %v", err)
+		}
+		defer os.Remove(f.Name())
+
+		f.Write([]byte("[client]\nuser=test\npassword=123456"))
+		context := NewMigrationContext()
+		context.ConfigFile = f.Name()
+		if err := context.ReadConfigFile(); err != nil {
+			t.Fatalf(".ReadConfigFile() failed: %v", err)
+		}
+
+		if context.config.Client.User != "test" {
+			t.Fatalf("Expected client user %q, got %q", "test", context.config.Client.User)
+		} else if context.config.Client.Password != "123456" {
+			t.Fatalf("Expected client password %q, got %q", "123456", context.config.Client.Password)
+		}
+	}
+	{
+		f, err := ioutil.TempFile("", t.Name())
+		if err != nil {
+			t.Fatalf("Failed to create tmp file: %v", err)
+		}
+		defer os.Remove(f.Name())
+
+		f.Write([]byte("[osc]\nmax_load=10"))
+		context := NewMigrationContext()
+		context.ConfigFile = f.Name()
+		if err := context.ReadConfigFile(); err != nil {
+			t.Fatalf(".ReadConfigFile() failed: %v", err)
+		}
+
+		if context.config.Osc.Max_Load != "10" {
+			t.Fatalf("Expected osc 'max_load' %q, got %q", "10", context.config.Osc.Max_Load)
+		}
+	}
+}

go/base/default_logger.go

@@ -1,3 +1,8 @@
+/*
+   Copyright 2022 GitHub Inc.
+         See https://github.com/github/gh-ost/blob/master/LICENSE
+*/
+
 package base
 
 import (
@@ -12,22 +17,18 @@ func NewDefaultLogger() *simpleLogger {
 
 func (*simpleLogger) Debug(args ...interface{}) {
 	log.Debug(args[0].(string), args[1:])
-	return
 }
 
 func (*simpleLogger) Debugf(format string, args ...interface{}) {
 	log.Debugf(format, args...)
-	return
 }
 
 func (*simpleLogger) Info(args ...interface{}) {
 	log.Info(args[0].(string), args[1:])
-	return
 }
 
 func (*simpleLogger) Infof(format string, args ...interface{}) {
 	log.Infof(format, args...)
-	return
 }
 
 func (*simpleLogger) Warning(args ...interface{}) error {
@@ -64,10 +65,8 @@ func (*simpleLogger) Fatale(err error) error {
 
 func (*simpleLogger) SetLevel(level log.LogLevel) {
 	log.SetLevel(level)
-	return
 }
 
 func (*simpleLogger) SetPrintStackTrace(printStackTraceFlag bool) {
 	log.SetPrintStackTrace(printStackTraceFlag)
-	return
 }

go/base/utils.go

@@ -1,5 +1,5 @@
 /*
-   Copyright 2016 GitHub Inc.
+   Copyright 2022 GitHub Inc.
 	 See https://github.com/github/gh-ost/blob/master/LICENSE
 */
 
@@ -25,9 +25,7 @@ func PrettifyDurationOutput(d time.Duration) string {
 	if d < time.Second {
 		return "0s"
 	}
-	result := fmt.Sprintf("%s", d)
-	result = prettifyDurationRegexp.ReplaceAllString(result, "")
-	return result
+	return prettifyDurationRegexp.ReplaceAllString(d.String(), "")
 }
 
 func FileExists(fileName string) bool {

go/binlog/gomysql_reader.go

@@ -1,5 +1,5 @@
 /*
-   Copyright 2016 GitHub Inc.
+   Copyright 2022 GitHub Inc.
 	 See https://github.com/github/gh-ost/blob/master/LICENSE
 */
 
@@ -64,7 +64,10 @@ func (this *GoMySQLReader) ConnectBinlogStreamer(coordinates mysql.BinlogCoordin
 	this.currentCoordinates = coordinates
 	this.migrationContext.Log.Infof("Connecting binlog streamer at %+v", this.currentCoordinates)
 	// Start sync with specified binlog file and position
-	this.binlogStreamer, err = this.binlogSyncer.StartSync(gomysql.Position{this.currentCoordinates.LogFile, uint32(this.currentCoordinates.LogPos)})
+	this.binlogStreamer, err = this.binlogSyncer.StartSync(gomysql.Position{
+		Name: this.currentCoordinates.LogFile,
+		Pos:  uint32(this.currentCoordinates.LogPos),
+	})
 
 	return err
 }

go/logic/hooks.go

@@ -1,6 +1,5 @@
 /*
-/*
-   Copyright 2016 GitHub Inc.
+   Copyright 2022 GitHub Inc.
 	 See https://github.com/github/gh-ost/blob/master/LICENSE
 */
 
@@ -72,9 +71,7 @@ func (this *HooksExecutor) applyEnvironmentVariables(extraVariables ...string) [
 	env = append(env, fmt.Sprintf("GH_OST_HOOKS_HINT_TOKEN=%s", this.migrationContext.HooksHintToken))
 	env = append(env, fmt.Sprintf("GH_OST_DRY_RUN=%t", this.migrationContext.Noop))
 
-	for _, variable := range extraVariables {
-		env = append(env, variable)
-	}
+	env = append(env, extraVariables...)
 	return env
 }
 

go/logic/inspect.go

@@ -805,5 +805,4 @@ func (this *Inspector) getReplicationLag() (replicationLag time.Duration, err er
 func (this *Inspector) Teardown() {
 	this.db.Close()
 	this.informationSchemaDb.Close()
-	return
 }

go/logic/migrator.go

@@ -1,5 +1,5 @@
 /*
-   Copyright 2016 GitHub Inc.
+   Copyright 2022 GitHub Inc.
 	 See https://github.com/github/gh-ost/blob/master/LICENSE
 */
 
@@ -176,16 +176,6 @@ func (this *Migrator) retryOperationWithExponentialBackoff(operation func() erro
 	return err
 }
 
-// executeAndThrottleOnError executes a given function. If it errors, it
-// throttles.
-func (this *Migrator) executeAndThrottleOnError(operation func() error) (err error) {
-	if err := operation(); err != nil {
-		this.throttler.throttle(nil)
-		return err
-	}
-	return nil
-}
-
 // consumeRowCopyComplete blocks on the rowCopyComplete channel once, and then
 // consumes and drops any further incoming events that may be left hanging.
 func (this *Migrator) consumeRowCopyComplete() {
@@ -823,78 +813,78 @@ func (this *Migrator) initiateStatus() error {
 // migration, and as response to the "status" interactive command.
 func (this *Migrator) printMigrationStatusHint(writers ...io.Writer) {
 	w := io.MultiWriter(writers...)
-	fmt.Fprintln(w, fmt.Sprintf("# Migrating %s.%s; Ghost table is %s.%s",
+	fmt.Fprintf(w, "# Migrating %s.%s; Ghost table is %s.%s\n",
 		sql.EscapeName(this.migrationContext.DatabaseName),
 		sql.EscapeName(this.migrationContext.OriginalTableName),
 		sql.EscapeName(this.migrationContext.DatabaseName),
 		sql.EscapeName(this.migrationContext.GetGhostTableName()),
-	))
-	fmt.Fprintln(w, fmt.Sprintf("# Migrating %+v; inspecting %+v; executing on %+v",
+	)
+	fmt.Fprintf(w, "# Migrating %+v; inspecting %+v; executing on %+v\n",
 		*this.applier.connectionConfig.ImpliedKey,
 		*this.inspector.connectionConfig.ImpliedKey,
 		this.migrationContext.Hostname,
-	))
-	fmt.Fprintln(w, fmt.Sprintf("# Migration started at %+v",
+	)
+	fmt.Fprintf(w, "# Migration started at %+v\n",
 		this.migrationContext.StartTime.Format(time.RubyDate),
-	))
+	)
 	maxLoad := this.migrationContext.GetMaxLoad()
 	criticalLoad := this.migrationContext.GetCriticalLoad()
-	fmt.Fprintln(w, fmt.Sprintf("# chunk-size: %+v; max-lag-millis: %+vms; dml-batch-size: %+v; max-load: %s; critical-load: %s; nice-ratio: %f",
+	fmt.Fprintf(w, "# chunk-size: %+v; max-lag-millis: %+vms; dml-batch-size: %+v; max-load: %s; critical-load: %s; nice-ratio: %f\n",
 		atomic.LoadInt64(&this.migrationContext.ChunkSize),
 		atomic.LoadInt64(&this.migrationContext.MaxLagMillisecondsThrottleThreshold),
 		atomic.LoadInt64(&this.migrationContext.DMLBatchSize),
 		maxLoad.String(),
 		criticalLoad.String(),
 		this.migrationContext.GetNiceRatio(),
-	))
+	)
 	if this.migrationContext.ThrottleFlagFile != "" {
 		setIndicator := ""
 		if base.FileExists(this.migrationContext.ThrottleFlagFile) {
 			setIndicator = "[set]"
 		}
-		fmt.Fprintln(w, fmt.Sprintf("# throttle-flag-file: %+v %+v",
+		fmt.Fprintf(w, "# throttle-flag-file: %+v %+v\n",
 			this.migrationContext.ThrottleFlagFile, setIndicator,
-		))
+		)
 	}
 	if this.migrationContext.ThrottleAdditionalFlagFile != "" {
 		setIndicator := ""
 		if base.FileExists(this.migrationContext.ThrottleAdditionalFlagFile) {
 			setIndicator = "[set]"
 		}
-		fmt.Fprintln(w, fmt.Sprintf("# throttle-additional-flag-file: %+v %+v",
+		fmt.Fprintf(w, "# throttle-additional-flag-file: %+v %+v\n",
 			this.migrationContext.ThrottleAdditionalFlagFile, setIndicator,
-		))
+		)
 	}
 	if throttleQuery := this.migrationContext.GetThrottleQuery(); throttleQuery != "" {
-		fmt.Fprintln(w, fmt.Sprintf("# throttle-query: %+v",
+		fmt.Fprintf(w, "# throttle-query: %+v\n",
 			throttleQuery,
-		))
+		)
 	}
 	if throttleControlReplicaKeys := this.migrationContext.GetThrottleControlReplicaKeys(); throttleControlReplicaKeys.Len() > 0 {
-		fmt.Fprintln(w, fmt.Sprintf("# throttle-control-replicas count: %+v",
+		fmt.Fprintf(w, "# throttle-control-replicas count: %+v\n",
 			throttleControlReplicaKeys.Len(),
-		))
+		)
 	}
 
 	if this.migrationContext.PostponeCutOverFlagFile != "" {
 		setIndicator := ""
 		if base.FileExists(this.migrationContext.PostponeCutOverFlagFile) {
 			setIndicator = "[set]"
 		}
-		fmt.Fprintln(w, fmt.Sprintf("# postpone-cut-over-flag-file: %+v %+v",
+		fmt.Fprintf(w, "# postpone-cut-over-flag-file: %+v %+v\n",
 			this.migrationContext.PostponeCutOverFlagFile, setIndicator,
-		))
+		)
 	}
 	if this.migrationContext.PanicFlagFile != "" {
-		fmt.Fprintln(w, fmt.Sprintf("# panic-flag-file: %+v",
+		fmt.Fprintf(w, "# panic-flag-file: %+v\n",
 			this.migrationContext.PanicFlagFile,
-		))
+		)
 	}
-	fmt.Fprintln(w, fmt.Sprintf("# Serving on unix socket: %+v",
+	fmt.Fprintf(w, "# Serving on unix socket: %+v\n",
 		this.migrationContext.ServeSocketFile,
-	))
+	)
 	if this.migrationContext.ServeTCPPort != 0 {
-		fmt.Fprintln(w, fmt.Sprintf("# Serving on TCP port: %+v", this.migrationContext.ServeTCPPort))
+		fmt.Fprintf(w, "# Serving on TCP port: %+v\n", this.migrationContext.ServeTCPPort)
 	}
 }
 
@@ -1195,7 +1185,6 @@ func (this *Migrator) iterateChunks() error {
 		// Enqueue copy operation; to be executed by executeWriteFuncs()
 		this.copyRowsQueue <- copyRowsFunc
 	}
-	return nil
 }
 
 func (this *Migrator) onApplyEventStruct(eventStruct *applyEventStruct) error {
@@ -1301,7 +1290,6 @@ func (this *Migrator) executeWriteFuncs() error {
 			}
 		}
 	}
-	return nil
 }
 
 // finalCleanup takes actions at very end of migration, dropping tables etc.