refactor: enhance golangci-lint linter and send notification when unexpected message existed

config/config.yaml

@@ -9,11 +9,6 @@ globalDefaultConfig: # global default settings, will be overridden by qbox org a
   golangcilintConfig: "config/linters-config/.golangci.yml" # golangci-lint config file to use
 
 customConfig: # custom config for specific orgs or repos
-  qbox: # github organization name
-    golangci-lint:
-      enable: true
-      args: ["run", "--allow-parallel-runners=true","--timeout=5m0s"]
-
   qbox/net-cache:
     golangci-lint:
       enable: true

config/linters-config/.golangci.yml

@@ -19,3 +19,11 @@ linters:
     - nlreturn # see https://github.com/qiniu/reviewbot/issues/148
     - wrapcheck # see https://github.com/qiniu/reviewbot/issues/180
     - gochecknoglobals # see https://github.com/qiniu/reviewbot/issues/182
+    - varnamelen # seems too arbitrary
+    - testpackage # seems too arbitrary
+    - depguard # seems too arbitrary
+    - cyclop # seems too arbitrary
+    - exhaustruct # seems too arbitrary
+    - execinquery # deprecated
+    - gomnd # deprecated
+

internal/linters/doc/note-check/note.go

@@ -74,7 +74,7 @@ func noteCheckFile(workdir, filename string) (map[string][]linters.LinterOutput,
 					continue
 				}
 
-				log.Infof("non-standard note: %s, pos: %v", line, fset.Position(cmt.Pos()))
+				log.Debugf("non-standard note: %s, pos: %v", line, fset.Position(cmt.Pos()))
 
 				v, ok := output[filename]
 				if !ok {

internal/linters/go/golangci_lint/golangci_lint.go

@@ -20,6 +20,9 @@ func golangciLintHandler(log *xlog.Logger, a linters.Agent) error {
 	if linters.IsEmpty(a.LinterConfig.Args...) {
 		// refer: https://github.com/qiniu/reviewbot/issues/146
 		a.LinterConfig.Args = append([]string{}, "run", "--timeout=5m0s", "--allow-parallel-runners=true")
+		// recommend to use the line-number format and disable the issued lines
+		// since these are more friendly to the reviewbot
+		a.LinterConfig.Args = append(a.LinterConfig.Args, "--out-format=line-number", "--print-issued-lines=false")
 	}
 
 	if a.LinterConfig.ConfigPath != "" {

internal/linters/linters.go

@@ -18,7 +18,6 @@
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"os"
 	"os/exec"
@@ -200,7 +199,7 @@
 			return err
 		}
 		log.Infof("[%s] create check run success, HTML_URL: %v", linterName, ch.GetHTMLURL())
-		if err := metric.SendAlertMessageIfNeeded(constructMKAlertMessage(linterName, a.PullRequestEvent.GetPullRequest().GetHTMLURL(), ch.GetHTMLURL(), lintResults)); err != nil {
+		if err := metric.NotifyWebhook(constructMessage(linterName, a.PullRequestEvent.GetPullRequest().GetHTMLURL(), ch.GetHTMLURL(), lintResults)); err != nil {
 			log.Errorf("failed to send alert message: %v", err)
 			// just log the error, not return
 		}
@@ -241,7 +240,7 @@
 			return err
 		}
 		log.Infof("[%s] add %d comments for this PR %d (%s) \n", linterName, len(addedCmts), num, orgRepo)
-		if err := metric.SendAlertMessageIfNeeded(constructMKAlertMessage(linterName, a.PullRequestEvent.GetPullRequest().GetHTMLURL(), addedCmts[0].GetHTMLURL(), lintResults)); err != nil {
+		if err := metric.NotifyWebhook(constructMessage(linterName, a.PullRequestEvent.GetPullRequest().GetHTMLURL(), addedCmts[0].GetHTMLURL(), lintResults)); err != nil {
 			log.Errorf("failed to send alert message: %v", err)
 			// just log the error, not return
 		}
@@ -259,14 +258,15 @@
 func Parse(log *xlog.Logger, output []byte, lineParser LineParser) (map[string][]LinterOutput, error) {
 	lines := strings.Split(string(output), "\n")
 
+	var unexpectedLines []string
 	var result = make(map[string][]LinterOutput)
 	for _, line := range lines {
 		if line == "" {
 			continue
 		}
 		output, err := lineParser(line)
 		if err != nil {
-			log.Debugf("unexpected linter check output: %v", line)
+			unexpectedLines = append(unexpectedLines, line)
 			continue
 		}
 
@@ -292,17 +292,43 @@
 		}
 	}
 
+	go func() {
+		var message string
+		for _, line := range unexpectedLines {
+			// trim the message to avoid too long
+			// wework webhook only support 4096 characters for markdown message
+			// see https://open.work.weixin.qq.com/api/doc/90000/90136/91770
+			if len(message) > 3000 {
+				message += "..."
+				break
+			}
+			message += line + "\n"
+		}
+
+		if message != "" {
+			message = fmt.Sprintf("unexpected lines:\n%v", message)
+			log.Warnf(message)
+			var msg metric.MessageBody
+			msg.MsgType = "markdown"
+			msg.Markdown.Content = fmt.Sprintf("<font color=\"warning\">[ATTENTION PLEASE]</font> %v", message)
+			// seed the alert message if the linter output is unexpected
+			// so that we can know the issue and fix it
+			if err := metric.NotifyWebhook(msg); err != nil {
+				log.Errorf("failed to send alert message: %v", err)
+				// just log the error, not return
+			}
+		}
+	}()
+
 	return result, nil
 }
 
 // common format LinterLine
 func GeneralLineParser(line string) (*LinterOutput, error) {
-	log.Debugf("parse line: %s", line)
 	pattern := `^(.*):(\d+):(\d+): (.*)$`
 	regex, err := regexp.Compile(pattern)
 	if err != nil {
-		log.Errorf("compile regex failed: %v", err)
-		return nil, err
+		return nil, fmt.Errorf("failed to compile regex: %v, err: %v", pattern, err)
 	}
 	matches := regex.FindStringSubmatch(line)
 	if len(matches) != 5 {
@@ -311,12 +337,12 @@
 
 	lineNumber, err := strconv.ParseInt(matches[2], 10, 64)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("unexpected line number: %s, err: %v, original line: %v", matches[2], err, line)
 	}
 
 	columnNumber, err := strconv.ParseInt(matches[3], 10, 64)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("unexpected column number: %s, err: %v, original line: %v", matches[3], err, line)
 	}
 
 	return &LinterOutput{
@@ -375,9 +401,9 @@
 	return count
 }
 
-func constructMKAlertMessage(linter, pr, link string, linterResults map[string][]LinterOutput) string {
+func constructMessage(linter, pr, link string, linterResults map[string][]LinterOutput) (msg metric.MessageBody) {
 	if len(linterResults) == 0 {
-		return ""
+		return
 	}
 
 	var message string
@@ -387,21 +413,7 @@
 		}
 	}
 
-	type mkMessage struct {
-		Msgtype string `json:"msgtype"`
-		Text    struct {
-			Content string `json:"content"`
-		} `json:"text"`
-	}
-
-	var m mkMessage
-	m.Msgtype = "text"
-	m.Text.Content = fmt.Sprintf("Linter: %v \nPR:   %v \nLink: %v \nDetails:\n%v\n", linter, pr, link, message)
-	b, err := json.Marshal(m)
-	if err != nil {
-		log.Errorf("failed to marshal message: %v", err)
-		return ""
-	}
-
-	return string(b)
+	msg.MsgType = "text"
+	msg.Text.Content = fmt.Sprintf("Linter: %v \nPR:   %v \nLink: %v \nDetails:\n%v\n", linter, pr, link, message)
+	return
 }

internal/linters/linters_test.go

@@ -17,7 +17,11 @@
 package linters
 
 import (
+	"fmt"
+	"reflect"
 	"testing"
+
+	"github.com/qiniu/reviewbot/internal/metric"
 )
 
 func TestFormatStaticcheckLine(t *testing.T) {
@@ -86,18 +90,18 @@ func TestIsEmpty(t *testing.T) {
 	}
 }
 
-func TestConstructMKAlertMessage(t *testing.T) {
+func TestConstructMessage(t *testing.T) {
 	tcs := []struct {
 		Linter        string
 		PR            string
 		Link          string
 		linterResults map[string][]LinterOutput
-		expected      string
+		expected      metric.MessageBody
 	}{
 		{
 			Linter: "golangci-lint",
-			PR:     "",
-			Link:   "",
+			PR:     "1",
+			Link:   "http://",
 			linterResults: map[string][]LinterOutput{
 				"cdn-admin.v2/client/dns/dnsapi.go": {
 					{
@@ -108,20 +112,25 @@ func TestConstructMKAlertMessage(t *testing.T) {
 					},
 				},
 			},
-			expected: `{"msgtype":"text","text":{"content":"Linter: golangci-lint \nPR:    \nLink:  \nDetails:\ncdn-admin.v2/client/dns/dnsapi.go:59:3: assignment to err\n\n"}}`,
+			expected: metric.MessageBody{
+				MsgType: "text",
+				Text: metric.MsgContent{
+					Content: fmt.Sprintf("Linter: %v \nPR:   %v \nLink: %v \nDetails:\n%v\n", "golangci-lint", "1", "http://", "cdn-admin.v2/client/dns/dnsapi.go:59:3: assignment to err\n"),
+				},
+			},
 		},
 		{
 			Linter:        "golangci-lint",
 			PR:            "",
 			Link:          "",
 			linterResults: map[string][]LinterOutput{},
-			expected:      "",
+			expected:      metric.MessageBody{},
 		},
 	}
 
 	for _, tc := range tcs {
-		actual := constructMKAlertMessage(tc.Linter, tc.PR, tc.Link, tc.linterResults)
-		if actual != tc.expected {
+		actual := constructMessage(tc.Linter, tc.PR, tc.Link, tc.linterResults)
+		if !reflect.DeepEqual(tc.expected, actual) {
 			t.Errorf("expected: %v, got: %v", tc.expected, actual)
 		}
 	}

internal/metric/metrics.go

@@ -1,6 +1,7 @@
 package metric
 
 import (
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"os"
@@ -27,22 +28,42 @@
 	issueCounter.WithLabelValues(repo, linter, pull_request, commit).Add(count)
 }
 
-// SendAlertMessageIfNeeded sends alert message to wework group if needed
+type MessageBody struct {
+	MsgType  string     `json:"msgtype"`
+	Text     MsgContent `json:"text,omitempty"`
+	Markdown MsgContent `json:"markdown,omitempty"`
+}
+
+type MsgContent struct {
+	Content string `json:"content"`
+}
+
+// NotifyWebhook sends message to wework group
 // refer: https://developer.work.weixin.qq.com/document/path/91770
-// The mkMessage is the markdown format message
-func SendAlertMessageIfNeeded(mkMessage string) error {
-	if WEWORK_WEBHOOK == "" || mkMessage == "" {
+func NotifyWebhook(message MessageBody) error {
+	if WEWORK_WEBHOOK == "" {
 		return nil
 	}
 
-	resp, err := http.DefaultClient.Post(WEWORK_WEBHOOK, "application/json", strings.NewReader(mkMessage))
+	body, err := json.Marshal(message)
+	if err != nil {
+		return err
+	}
+
+	resp, err := http.DefaultClient.Post(WEWORK_WEBHOOK, "application/json", strings.NewReader(string(body)))
 	if err != nil {
 		return err
 	}
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		return fmt.Errorf("send alert message failed: %v", resp)
+		return fmt.Errorf("send message failed: %v", resp)
 	}
+
+	errCode := resp.Header.Get("Error-Code")
+	if errCode != "0" {
+		return fmt.Errorf("send message failed, errCode: %v, errMsg: %v", errCode, resp.Header.Get("Error-Msg"))
+	}
+
 	return nil
 }