*: add lint CI (#99)

.github/workflows/common.yml

@@ -58,7 +58,9 @@ jobs:
           key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
           restore-keys: |
             ${{ runner.os }}-go-
-      - run: make ${{ inputs.target }}
+      - name: make ${{ inputs.target }} 
+        if: ${{ inputs.target != '' }}
+        run: make ${{ inputs.target }}
       - name: "set up tmate session if necessary"
         if: ${{ failure() && inputs.debug }}
         uses: mxschmitt/action-tmate@v3

.github/workflows/main.yml

@@ -28,6 +28,7 @@ concurrency:
 
 jobs:
   cmd:
+    if: ${{ github.event_name != 'push' }}
     uses: ./.github/workflows/common.yml
     with:
       debug: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.debug }}
@@ -36,17 +37,36 @@ jobs:
       all_platform: true
 
   build:
+    if: ${{ github.event_name != 'push' }}
     needs: cmd
     uses: ./.github/workflows/common.yml
     with:
       debug: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.debug }}
       ref: ${{ inputs.ref || github.ref }}
       target: "build"
 
+  lint:
+    if: ${{ github.event_name != 'push' }}
+    needs: build
+    uses: ./.github/workflows/common.yml
+    with:
+      debug: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.debug }}
+      ref: ${{ inputs.ref || github.ref }}
+      target: "lint"
+
   test:
+    if: ${{ github.event_name != 'push' }}
     needs: build
     uses: ./.github/workflows/common.yml
     with:
       debug: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.debug }}
       ref: ${{ inputs.ref || github.ref }}
       target: "test"
+
+  cache:
+    if: ${{ github.event_name == 'push' }}
+    uses: ./.github/workflows/common.yml
+    with:
+      ref: ${{ inputs.ref || github.ref }}
+      debug: false
+      target: "build lint test"

.golangci.yaml

@@ -1,99 +1,30 @@
-# options for analysis running
 run:
-  # default concurrency is a available CPU number
-  concurrency: 4
-
-  # timeout for analysis, e.g. 30s, 5m, default is 1m
-  timeout: 20m
-
-  # exit code when at least one issue was found, default is 1
+  timeout: 10m
   issues-exit-code: 1
 
-  # include test files or not, default is true
-  tests: false
-
-  # list of build tags, all linters use it. Default is empty list.
-  build-tags:
-
-  # default is true. Enables skipping of directories:
-  #   vendor$, third_party$, testdata$, examples$, Godeps$, builtin$
-  skip-dirs-use-default: true
-
-  # which dirs to skip: they won't be analyzed;
-  # can use regexp here: generated.*, regexp is applied on full path;
-  # default value is empty list, but next dirs are always skipped independently
-  # from this option's value:
-  #   	vendor$, third_party$, testdata$, examples$, Godeps$, builtin$
-  #  skip-dirs:
-  #    - ^test.*
-
-  # by default isn't set. If set we pass it to "go list -mod={option}". From "go help modules":
-  # If invoked with -mod=readonly, the go command is disallowed from the implicit
-  # automatic updating of go.mod described above. Instead, it fails when any changes
-  # to go.mod are needed. This setting is most useful to check that go.mod does
-  # not need updates, such as in a continuous integration and testing system.
-  # If invoked with -mod=vendor, the go command assumes that the vendor
-  # directory holds the correct copies of dependencies and ignores
-  # the dependency descriptions in go.mod.
-  modules-download-mode: readonly
-
-  # which files to skip: they will be analyzed, but issues from them
-  # won't be reported. Default value is empty list, but there is
-  # no need to include all autogenerated files, we confidently recognize
-  # autogenerated files. If it's not please let us know.
-  skip-files:
-  #  - ".*\\.my\\.go$"
-  #  - lib/bad.go
-
-# all available settings of specific linters
 linters-settings:
-  misspell:
-    # Correct spellings using locale preferences for US or UK.
-    # Default is to use a neutral variety of English.
-    # Setting locale to US will correct the British spelling of 'colour' to 'color'.
-    # locale: US
-    ignore-words:
-      - rela # This is for elf.SHT_RELA
+  errcheck:
+    exclude-functions:
+        - io.WriteString
+        - (*go.uber.org/zap/buffer.Buffer).WriteString
+        - (*go.uber.org/zap/buffer.Buffer).WriteByte
 
 issues:
-  # Excluding configuration per-path, per-linter, per-text and per-source
   exclude-rules:
-    - linters: [staticcheck]
-      text: "SA1019"                  # this is rule for deprecated method
-    - linters: [staticcheck]
-      text: "SA9003: empty branch"
-    - linters: [staticcheck]
-      text: "SA2001: empty critical section"
-    - linters: [goerr113]
-      text: "do not define dynamic errors, use wrapped static errors instead" # This rule to avoid opinionated check fmt.Errorf("text")
-    - path: _test\.go                # Skip 1.13 errors check for test files
+    - path: _test\.go
+      linters:
+        - gosec
+    - path: util/security/tls.go
       linters:
-        - goerr113
+        - gosec
+      text: "G402:"
+    - linters:
+        - unused
+      source: "updateAuthInfoFromSessionStates"
+
 linters:
-  disable-all: true
   enable:
-    - deadcode
-    - goerr113
+    - gosec
     - gofmt
     - goimports
-    - gosimple
-    - govet
-    - ineffassign
     - misspell
-    - staticcheck
-    - stylecheck
-    - varcheck
-
-# To enable later if makes sense
-#    - errcheck
-#    - gocyclo
-#    - golint
-#    - gosec
-#    - gosimple
-#    - lll
-#    - maligned
-#    - misspell
-#    - prealloc
-#    - structcheck
-#    - typecheck
-#    - unused

Makefile

@@ -29,7 +29,7 @@ endif
 IMAGE_TAG ?= latest
 EXECUTABLE_TARGETS := $(patsubst cmd/%,cmd_%,$(wildcard cmd/*))
 
-.PHONY: cmd_% build test docker
+.PHONY: cmd_% build test docker ./bin/golangci-lint ./bin/gocovmerge
 
 default: cmd
 
@@ -44,14 +44,21 @@ cmd_%: SOURCE=$(patsubst cmd_%,./cmd/%,$@)
 cmd_%:
 	go build $(BUILDFLAGS) -o $(OUTPUT) $(SOURCE)
 
+lint: ./bin/golangci-lint
+	$(GOBIN)/golangci-lint run
+	cd lib && $(GOBIN)/golangci-lint run
+
 test: ./bin/gocovmerge
 	rm -f .cover.*
 	go test -coverprofile=.cover.pkg ./...
 	cd lib && go test -coverprofile=../.cover.lib ./...
-	./bin/gocovmerge .cover.* > .cover
+	$(GOBIN)/gocovmerge .cover.* > .cover
 	rm -f .cover.*
 	go tool cover -html=.cover -o .cover.html
 
+./bin/golangci-lint:
+	GOBIN=$(GOBIN) go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest
+
 ./bin/gocovmerge:
 	GOBIN=$(GOBIN) go install github.com/wadey/gocovmerge@master
 

cmd/tiproxy/main.go

@@ -16,7 +16,6 @@
 package main
 
 import (
-	"io/ioutil"
 	"os"
 
 	"github.com/pingcap/TiProxy/lib/config"
@@ -39,7 +38,7 @@ func main() {
 	logLevel := rootCmd.PersistentFlags().String("log_level", "", "log level")
 
 	rootCmd.RunE = func(cmd *cobra.Command, _ []string) error {
-		proxyConfigData, err := ioutil.ReadFile(*configFile)
+		proxyConfigData, err := os.ReadFile(*configFile)
 		if err != nil {
 			return err
 		}

lib/cli/main.go

@@ -19,9 +19,9 @@ import (
 	"net/http"
 
 	"github.com/pingcap/TiProxy/lib/config"
+	"github.com/pingcap/TiProxy/lib/util/cmd"
 	"github.com/pingcap/TiProxy/lib/util/security"
 	"github.com/spf13/cobra"
-	"go.uber.org/zap"
 )
 
 func GetRootCmd(tlsConfig *tls.Config) *cobra.Command {
@@ -42,12 +42,12 @@ func GetRootCmd(tlsConfig *tls.Config) *cobra.Command {
 	keyPath := rootCmd.PersistentFlags().String("key", "", "key for server-side client authentication")
 	rootCmd.PersistentFlags().Bool("indent", true, "whether indent the returned json")
 	rootCmd.PersistentPreRunE = func(_ *cobra.Command, _ []string) error {
-		zapcfg := zap.NewDevelopmentConfig()
-		zapcfg.Encoding = *logEncoder
-		if level, err := zap.ParseAtomicLevel(*logLevel); err == nil {
-			zapcfg.Level = level
-		}
-		logger, err := zapcfg.Build()
+		logger, _, _, err := cmd.BuildLogger(&config.Log{
+			Encoder: *logEncoder,
+			LogOnline: config.LogOnline{
+				Level: *logLevel,
+			},
+		})
 		if err != nil {
 			return err
 		}

lib/cli/namespace.go

@@ -18,7 +18,6 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
-	"io/ioutil"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -34,7 +33,7 @@ const (
 )
 
 func listAllFiles(dir, ext string) ([]string, error) {
-	infos, err := ioutil.ReadDir(dir)
+	infos, err := os.ReadDir(dir)
 	if err != nil {
 		return nil, err
 	}
@@ -178,7 +177,7 @@ func GetNamespaceCmd(ctx *Context) *cobra.Command {
 			}
 
 			for _, nFile := range nFiles {
-				fileData, err := ioutil.ReadFile(nFile)
+				fileData, err := os.ReadFile(nFile)
 				if err != nil {
 					return err
 				}

lib/cli/util.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"math/rand"
 	"net/http"
 
@@ -70,7 +69,7 @@ func doRequest(ctx context.Context, bctx *Context, method string, url string, rd
 				return "", err
 			}
 		}
-		resb, _ := ioutil.ReadAll(res.Body)
+		resb, _ := io.ReadAll(res.Body)
 		res.Body.Close()
 
 		switch res.StatusCode {

lib/go.mod

@@ -18,4 +18,5 @@ require (
 	github.com/pkg/errors v0.9.1 // indirect
 	golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1 // indirect
 	gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect
+	gopkg.in/natefinch/lumberjack.v2 v2.0.0
 )

lib/util/cmd/cmd.go

@@ -19,27 +19,12 @@ import (
 	"fmt"
 	"os"
 	"os/signal"
-	"sync"
 	"syscall"
 
-	"github.com/pingcap/log"
 	"github.com/spf13/cobra"
-	"go.uber.org/zap"
-	"go.uber.org/zap/zapcore"
 )
 
-var registerEncoders sync.Once
-
 func RunRootCommand(rootCmd *cobra.Command) {
-	registerEncoders.Do(func() {
-		zap.RegisterEncoder("tidb", func(cfg zapcore.EncoderConfig) (zapcore.Encoder, error) {
-			return log.NewTextEncoder(&log.Config{})
-		})
-		zap.RegisterEncoder("newtidb", func(cfg zapcore.EncoderConfig) (zapcore.Encoder, error) {
-			return NewTiDBEncoder(cfg), nil
-		})
-	})
-
 	ctx, cancel := context.WithCancel(context.Background())
 	go func() {
 		sc := make(chan os.Signal, 1)

lib/util/cmd/encoder.go

@@ -148,20 +148,6 @@ func (e *tidbEncoder) EncodeEntry(ent zapcore.Entry, fields []zapcore.Field) (*b
 }
 
 /* map encoder part */
-func (f *tidbEncoder) needDoubleQuotes(s string) bool {
-	for i := 0; i < len(s); {
-		b := s[i]
-		if b <= 0x20 {
-			return true
-		}
-		switch b {
-		case '\\', '"', '[', ']', '=':
-			return true
-		}
-		i++
-	}
-	return false
-}
 func (f *tidbEncoder) safeAddString(s string) {
 	needQuotes := false
 outerloop: