Merge pull request #69 from planetscale/lint-more

cleanup: make sure to follow linter rules

Author: rohit-nayak-ps

.github/workflows/ci.yml

@@ -7,39 +7,58 @@ jobs:
   test:
     runs-on: ubuntu-latest
     steps:
-      - uses: actions/checkout@v4
+      - name: Checkout code
+        uses: actions/checkout@v4
 
-      - uses: actions/setup-go@v5
+      - name: Set up Go
+        uses: actions/setup-go@v5
         with:
           go-version-file: './go.mod'
 
-      - run: go version
+      - name: Cache Go modules
+        uses: actions/cache@v3
+        with:
+          path: |
+            ~/.cache/go-build
+            ~/go/pkg/mod
+          key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
+          restore-keys: |
+            ${{ runner.os }}-go-
+
+      - name: Install Dependencies
+        run: go mod download
 
-      - name: Go mod tidy
+      - name: Verify go.mod and go.sum
         run: |
           go mod tidy
           git diff --exit-code || (echo "go.mod or go.sum is not clean! Run 'go mod tidy' and commit the changes." && exit 1)
 
-      - name: Go Vet
-        run: go vet ./go/...
+      - name: Install Tools
+        run: |
+          make install-tools
+
+      - name: Display Go version
+        run: go version
 
       - name: Build
         run: make build
 
-      - name: Check formatting
+      - name: Check Formatting
         run: |
           make pretty
           git diff --exit-code || (echo "Code is not formatted correctly! Run 'make pretty' and commit the changes." && exit 1)
 
+      - name: Run Linting
+        run: make lint
+
       - name: Install go-junit-report
         run: go install github.com/jstemmer/go-junit-report@latest
 
       - name: Run Tests and Convert to JUnit
-        run: go test -v ./go/... | go-junit-report > report.xml
+        run: go test -v ./... | go-junit-report > report.xml
 
       - name: Test Summary
-        if: true
-        uses: test-summary/action@31493c76ec9e7aa675f1585d3ed6f1da69269a86 # v2.4
+        uses: test-summary/[email protected]
         with:
           paths: "report.xml"
-          show: "fail"
+          show: "fail"

.gitignore

@@ -26,4 +26,6 @@ errors/
 /vt
 
 # Do not ignore anything inside the src/vitess-tester directory
-!/go
+!/go
+
+go/testdata/expected/

.golangci.yml

@@ -131,6 +131,7 @@ linters-settings:
   gosec:
     excludes:
       - G115
+      - G404
 
   inamedparam:
     # Skips check for interface methods with only a single parameter.

Makefile

@@ -1,12 +1,20 @@
 .PHONY: all build test tidy clean pretty install-tools lint install-hooks
+.DEFAULT_GOAL := test_and_build
 
-GO := go
 REQUIRED_GO_VERSION := 1.23
-GOLANGCI_LINT_VERSION := v1.55.2
+GOLANGCI_LINT_VERSION := v1.62.0
+
+# Determine the Go binary directory
+GOBIN_DIR := $(or $(GOBIN), $(shell go env GOBIN))
+ifeq ($(GOBIN_DIR),)
+	GOBIN_DIR := $(shell go env GOPATH)/bin
+endif
+
+test_and_build: test build
 
 # Version check
 check_version:
-	@GO_VERSION=$$($(GO) version | awk '{print $$3}' | sed 's/go//'); \
+	@GO_VERSION=$$(go version | awk '{print $$3}' | sed 's/go//'); \
 	MAJOR_VERSION=$$(echo $$GO_VERSION | cut -d. -f1); \
 	MINOR_VERSION=$$(echo $$GO_VERSION | cut -d. -f2); \
 	if [ "$$MAJOR_VERSION" -eq 1 ] && [ "$$MINOR_VERSION" -lt 23 ]; then \
@@ -19,41 +27,47 @@ check_version:
 default: check_version build
 
 build:
-	$(GO) build -o vt ./go/vt
+	go build -o vt ./go/vt
 
 test:
-	$(GO) test -count=1 ./go/...
+	go test -count=1 ./go/...
 
 tidy:
-	$(GO) mod tidy
+	go mod tidy
 
 clean:
-	$(GO) clean -i ./...
+	go clean -i ./...
 	rm -f vt
 
 # Pretty: formats the code using gofumpt and goimports-reviser
-pretty: install-tools
+pretty: check-tools
 	@echo "Running formatting tools..."
-	@gofumpt -l -w . >/dev/null 2>&1 || true
-	@goimports-reviser -project-name $$(go list -m) -rm-unused -set-alias -format . >/dev/null 2>&1 || true
+	@gofumpt -w . >/dev/null 2>&1
+	@goimports-reviser -recursive -project-name $$(go list -m) -rm-unused -set-alias ./go >/dev/null 2>&1
 
-# Install tools: Checks if the required tools are installed, installs if missing
+# Tools installation command
 install-tools:
-	@command -v gofumpt >/dev/null 2>&1 || { \
-		echo "Installing gofumpt..."; \
-		go install mvdan.cc/gofumpt@latest >/dev/null 2>&1; \
-	}
-	@command -v goimports-reviser >/dev/null 2>&1 || { \
-		echo "Installing goimports-reviser..."; \
-		go install github.com/incu6us/goimports-reviser@latest >/dev/null 2>&1; \
-	}
-	@command -v golangci-lint >/dev/null 2>&1 || { \
-		echo "Installing golangci-lint..."; \
-		curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $$(go env GOPATH)/bin $(GOLANGCI_LINT_VERSION); \
-	}
+	@echo "Installing gofumpt..."
+	go install mvdan.cc/gofumpt@latest
+
+	@echo "Installing goimports-reviser..."
+	go install github.com/incu6us/goimports-reviser/v3@latest
+
+	@echo "Installing golangci-lint..."
+	curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | \
+		sh -s -- -b $(GOBIN_DIR) $(GOLANGCI_LINT_VERSION)
+
+	@echo "All tools installed successfully."
+
+# Ensure tools are available
+check-tools:
+	@command -v gofumpt >/dev/null 2>&1 || { echo "gofumpt not found. Run 'make install-tools' to install it." >&2; exit 1; }
+	@command -v goimports-reviser >/dev/null 2>&1 || { echo "goimports-reviser not found. Run 'make install-tools' to install it." >&2; exit 1; }
+	@command -v golangci-lint >/dev/null 2>&1 || { echo "golangci_lint not found. Run 'make install-tools' to install it." >&2; exit 1; }
+
 
 # Lint: runs golangci-lint
-lint: install-tools
+lint: check-tools
 	@echo "Running golangci-lint..."
 	@golangci-lint run --config .golangci.yml ./go/...
 

git-hooks/pre-commit

@@ -18,12 +18,12 @@ for file in $GO_FILES; do
     }
 done
 
-# 2. Run goimports-reviser to verify formatting on changed files (without writing changes)
-echo "Checking goimports-reviser formatting..."
+# Check imports with goimports-reviser
+echo "Checking goimports-reviser imports..."
 for file in $GO_FILES; do
-    goimports-reviser -project-name $(go list -m) -rm-unused -set-alias -format "$file" -diff >/dev/null 2>&1
+    goimports-reviser -project-name $(go list -m) -rm-unused -set-alias "$file" >/dev/null 2>&1
     if [[ $? -ne 0 ]]; then
-        echo "Error: Imports not correctly formatted in $file. Please run 'make pretty'."
+        echo "Error: Imports not correctly organized in $file. Please run 'make pretty'."
         exit 1
     fi
 done

go/data/vtgate_log_parse.go

@@ -167,13 +167,22 @@ func getBindVariables(bindVarsRaw string, lineNumber int) (map[string]*querypb.B
 		var val []byte
 		switch {
 		case sqltypes.IsIntegral(bvType) || sqltypes.IsFloat(bvType):
-			val = []byte(strconv.FormatFloat(value.Value.(float64), 'f', -1, 64))
+			f, ok := value.Value.(float64)
+			if ok {
+				val = []byte(strconv.FormatFloat(f, 'f', -1, 64))
+			}
 		case bvType == sqltypes.Tuple:
 			// the query log of vtgate does not list all the values for a tuple
 			// instead it lists the following: "v2": {"type": "TUPLE", "value": "2 items"}
 			return nil, fmt.Errorf("line %d: cannot parse tuple bind variables", lineNumber)
-		default:
-			val = []byte(value.Value.(string))
+		}
+		if val == nil {
+			sval, ok := value.Value.(string)
+			if !ok {
+				return nil, fmt.Errorf("line %d: cannot parse bind variable value", lineNumber)
+			}
+
+			val = []byte(sval)
 		}
 		bvProcessed[key] = &querypb.BindVariable{
 			Type:  bvType,

go/summarize/reading.go

@@ -58,14 +58,18 @@ func getDecoderAndDelim(file *os.File) (*json.Decoder, json.Delim) {
 	if err != nil {
 		exit("Error reading json: " + err.Error())
 	}
+	delim, ok := val.(json.Delim)
+	if !ok {
+		exit("Error reading json: expected delimiter")
+	}
 
 	// Reset the file pointer to the beginning
 	_, err = file.Seek(0, io.SeekStart)
 	if err != nil {
 		exit("Error rewinding file: " + err.Error())
 	}
 	decoder = json.NewDecoder(file)
-	return decoder, val.(json.Delim)
+	return decoder, delim
 }
 
 func readTracedQueryFile(decoder *json.Decoder, fileName string) readingSummary {

go/summarize/summarize-keys.go

@@ -177,7 +177,7 @@ func printKeysSummary(out io.Writer, file readingSummary, now time.Time, hotMetr
 	summary := summarizeKeysQueries(file.AnalysedQueries, metricReader, schemaInfo)
 
 	renderHotQueries(md, summary.hotQueries, metricReader)
-	renderTableUsage(summary.tables, md)
+	renderTableUsage(summary.tables, md, schemaInfo != nil)
 	renderTablesJoined(md, file.AnalysedQueries)
 	renderFailures(md, summary.failures)
 
@@ -268,7 +268,7 @@ func renderHotQueries(md *markdown.MarkDown, queries []keys.QueryAnalysisResult,
 	}
 }
 
-func renderTableUsage(tableSummaries []TableSummary, md *markdown.MarkDown) {
+func renderTableUsage(tableSummaries []TableSummary, md *markdown.MarkDown, includeRowCount bool) {
 	if len(tableSummaries) == 0 {
 		return
 	}
@@ -278,24 +278,31 @@ func renderTableUsage(tableSummaries []TableSummary, md *markdown.MarkDown) {
 	})
 
 	md.PrintHeader("Tables", 2)
-	renderTableOverview(md, tableSummaries)
+	renderTableOverview(md, tableSummaries, includeRowCount)
 
 	md.PrintHeader("Column Usage", 3)
 	for _, summary := range tableSummaries {
 		renderColumnUsageTable(md, summary)
 	}
 }
 
-func renderTableOverview(md *markdown.MarkDown, tableSummaries []TableSummary) {
-	headers := []string{"Table Name", "Reads", "Writes", "Number of Rows"}
+func renderTableOverview(md *markdown.MarkDown, tableSummaries []TableSummary, includeRowCount bool) {
+	headers := []string{"Table Name", "Reads", "Writes"}
+	if includeRowCount {
+		headers = append(headers, "Number of Rows")
+	}
 	var rows [][]string
 	for _, summary := range tableSummaries {
-		rows = append(rows, []string{
+		thisRow := []string{
 			summary.Table,
 			strconv.Itoa(summary.ReadQueryCount),
 			strconv.Itoa(summary.WriteQueryCount),
-			strconv.Itoa(summary.RowCount),
-		})
+		}
+		if includeRowCount {
+			thisRow = append(thisRow, strconv.Itoa(summary.RowCount))
+		}
+
+		rows = append(rows, thisRow)
 	}
 	md.PrintTable(headers, rows)
 }

go/summarize/summarize-keys_test.go

@@ -96,7 +96,7 @@ func TestSummarizeKeysWithHotnessFile(t *testing.T) {
 			require.NoError(t, err)
 			sb := &strings.Builder{}
 			now := time.Date(2024, time.January, 1, 1, 2, 3, 0, time.UTC)
-			printKeysSummary(sb, file, now, metric, "../testdata/bigger_slow_query_schema_info.json")
+			printKeysSummary(sb, file, now, metric, "")
 			expected, err := os.ReadFile(fmt.Sprintf("../testdata/bigger_slow_log_%s.md", metric))
 			require.NoError(t, err)
 			assert.Equal(t, string(expected), sb.String())

go/summarize/utils.go

@@ -32,7 +32,7 @@ const (
 	dbInfoFile
 )
 
-var fileTypeMap = map[string]fileType{
+var fileTypeMap = map[string]fileType{ //nolint:gochecknoglobals // this is instead of a const
 	"trace":  traceFile,
 	"keys":   keysFile,
 	"dbinfo": dbInfoFile,
@@ -43,47 +43,50 @@ var fileTypeMap = map[string]fileType{
 func getFileType(filename string) (fileType, error) {
 	file, err := os.Open(filename)
 	if err != nil {
-		return unknownFile, errors.New(fmt.Sprintf("error opening file: %v", err))
+		return unknownFile, fmt.Errorf("error opening file: %v", err)
 	}
 	defer file.Close()
 
 	decoder := json.NewDecoder(file)
 
 	token, err := decoder.Token()
 	if err != nil {
-		return unknownFile, errors.New(fmt.Sprintf("Error reading token: %v", err))
+		return unknownFile, fmt.Errorf("error reading token: %v", err)
 	}
 
 	if delim, ok := token.(json.Delim); !ok || delim != '{' {
-		return unknownFile, errors.New(fmt.Sprintf("Expected start of object '{'"))
+		return unknownFile, errors.New("expected start of object '{'")
 	}
 
-	for decoder.More() {
-		keyToken, err := decoder.Token()
-		if err != nil {
-			return unknownFile, errors.New(fmt.Sprintf("Error reading key token: %v", err))
-		}
+	if !decoder.More() {
+		return unknownFile, nil
+	}
 
-		key, ok := keyToken.(string)
-		if !ok {
-			return unknownFile, errors.New(fmt.Sprintf("Expected key to be a string: %s", keyToken))
-		}
+	keyToken, err := decoder.Token()
+	if err != nil {
+		return unknownFile, fmt.Errorf("error reading key token: %v", err)
+	}
 
-		valueToken, err := decoder.Token()
-		if err != nil {
-			return unknownFile, errors.New(fmt.Sprintf("Error reading value token: %v", err))
-		}
+	key, ok := keyToken.(string)
+	if !ok {
+		return unknownFile, fmt.Errorf("expected key to be a string: %s", keyToken)
+	}
+
+	valueToken, err := decoder.Token()
+	if err != nil {
+		return unknownFile, fmt.Errorf("error reading value token: %v", err)
+	}
 
-		if key == "fileType" {
-			if fileType, ok := fileTypeMap[valueToken.(string)]; ok {
-				return fileType, nil
-			} else {
-				return unknownFile, errors.New(fmt.Sprintf("Unknown FileType: %s", valueToken))
-			}
-		} else {
-			// Currently we expect the first key to be FileType, for optimization reasons
-			return unknownFile, nil
+	if key == "fileType" {
+		s, ok := valueToken.(string)
+		if !ok {
+			return unknownFile, fmt.Errorf("expected value to be a string: %s", valueToken)
+		}
+		if fileType, ok := fileTypeMap[s]; ok {
+			return fileType, nil
 		}
+		return unknownFile, fmt.Errorf("unknown FileType: %s", valueToken)
 	}
+
 	return unknownFile, nil
 }