diff --git a/config/config.go b/config/config.go index dfd37716..30f9064f 100644 --- a/config/config.go +++ b/config/config.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "os" "sigs.k8s.io/yaml" @@ -11,10 +12,14 @@ type Config map[string]map[string]Linter type Linter struct { // Enable is whether to enable this linter, if false, linter still run but not report. - Enable *bool `json:"enable,omitempty"` - WorkDir string `json:"workDir,omitempty"` - Command string `json:"command,omitempty"` - Args []string `json:"args,omitempty"` + Enable *bool `json:"enable,omitempty"` + // WorkDir is the working directory of the linter. + WorkDir string `json:"workDir,omitempty"` + // Command is the command to run the linter. e.g. "golangci-lint", "staticcheck" + // If empty, use the linter name as the command. + Command string `json:"command,omitempty"` + // Args is the arguments of the command. + Args []string `json:"args,omitempty"` // ReportFormat is the format of the report, if empty, use github_checks by default. // e.g. "github_checks", "github_pr_review" @@ -24,6 +29,10 @@ type Linter struct { ReportFormat string `json:"reportFormat,omitempty"` } +func (l Linter) String() string { + return fmt.Sprintf("Linter{Enable: %v, WorkDir: %v, Command: %v, Args: %v, ReportFormat: %v}", *l.Enable, l.WorkDir, l.Command, l.Args, l.ReportFormat) +} + // NewConfig returns a new Config. func NewConfig(conf string) (Config, error) { var c Config @@ -36,7 +45,7 @@ func NewConfig(conf string) (Config, error) { return nil, err } - c = fixConfig(c) + c = FixConfig(c) return c, nil } @@ -53,24 +62,37 @@ func (c Config) CustomLinterConfigs(org, repo string) map[string]Linter { return nil } -// fixConfig fix the config +// FixConfig fix the config // 1. if linterConfig.Enable is nil, set it to true // 2. if linterConfig.ReportFormat is empty, set it to "github_checks" -func fixConfig(c Config) Config { - for org, repoConfig := range c { - for repo, linterConfig := range repoConfig { - if linterConfig.Enable == nil { - enable := true - linterConfig.Enable = &enable - } - - if linterConfig.ReportFormat == "" { - linterConfig.ReportFormat = "github_checks" - } - - c[org][repo] = linterConfig +// 3. if linterConfig.Command is empty, set it to linterName +func FixConfig(c Config) Config { + for repo, repoConfig := range c { + for linterName, linterConfig := range repoConfig { + c[repo][linterName] = FixLinterConfig(linterConfig, linterName) } } return c } + +// FixLinterConfig fix the linter config +func FixLinterConfig(linterConfig Linter, linterName string) Linter { + // if linterConfig.Enable is nil, set it to true + if linterConfig.Enable == nil { + enable := true + linterConfig.Enable = &enable + } + + // if linterConfig.ReportFormat is empty, set it to "github_checks" + if linterConfig.ReportFormat == "" { + linterConfig.ReportFormat = "github_checks" + } + + // if linterConfig.Command is empty, set it to linterName + if linterConfig.Command == "" { + linterConfig.Command = linterName + } + + return linterConfig +} diff --git a/internal/github/github.go b/internal/linters/github.go similarity index 95% rename from internal/github/github.go rename to internal/linters/github.go index a4b80a63..a080674b 100644 --- a/internal/github/github.go +++ b/internal/linters/github.go @@ -14,7 +14,7 @@ limitations under the License. */ -package github +package linters import ( "context" @@ -24,7 +24,6 @@ import ( "time" "github.com/google/go-github/v57/github" - "github.com/qiniu/reviewbot/internal/linters" "github.com/qiniu/x/log" ) @@ -139,7 +138,7 @@ func GetCommitIDFromContentsURL(contentsURL string) (string, error) { return matches[1], nil } -func BuildPullRequestCommentBody(linterName string, lintErrs map[string][]linters.LinterOutput, commitFiles []*github.CommitFile) ([]*github.PullRequestComment, error) { +func BuildPullRequestCommentBody(linterName string, lintErrs map[string][]LinterOutput, commitFiles []*github.CommitFile) ([]*github.PullRequestComment, error) { var comments []*github.PullRequestComment hunkChecker, err := NewGithubCommitFileHunkChecker(commitFiles) if err != nil { @@ -172,7 +171,7 @@ func BuildPullRequestCommentBody(linterName string, lintErrs map[string][]linter } message := fmt.Sprintf("[%s] %s\n%s", - linterName, lintErr.Message, linters.CommentFooter) + linterName, lintErr.Message, CommentFooter) comments = append(comments, &github.PullRequestComment{ Body: github.String(message), diff --git a/internal/linters/go/staticcheck/staticcheck.go b/internal/linters/go/staticcheck/staticcheck.go index 5bc221ce..02cd66d0 100644 --- a/internal/linters/go/staticcheck/staticcheck.go +++ b/internal/linters/go/staticcheck/staticcheck.go @@ -17,124 +17,22 @@ package staticcheck import ( - "context" - "os/exec" - - gh "github.com/qiniu/reviewbot/internal/github" "github.com/qiniu/reviewbot/internal/linters" - "github.com/qiniu/x/log" "github.com/qiniu/x/xlog" ) -var lintName = "staticcheck" +// refer to https://staticcheck.io/docs/ +const linterName = "staticcheck" func init() { - linters.RegisterPullRequestHandler(lintName, staticcheckHandler) + linters.RegisterPullRequestHandler(linterName, staticcheckHandler) } func staticcheckHandler(log *xlog.Logger, a linters.Agent) error { - var ( - org = a.PullRequestEvent.GetRepo().GetOwner().GetLogin() - repo = a.PullRequestEvent.GetRepo().GetName() - num = a.PullRequestEvent.GetNumber() - orgRepo = org + "/" + repo - ) - - executor, err := NewStaticcheckExecutor(a.LinterConfig.WorkDir) - if err != nil { - log.Errorf("init staticcheck executor failed: %v", err) - return err - } - if linters.IsEmpty(a.LinterConfig.Args...) { // turn off compile errors by default a.LinterConfig.Args = append([]string{}, "-debug.no-compile-errors=true", "./...") } - output, err := executor.Run(log, a.LinterConfig.Args...) - if err != nil { - log.Errorf("staticcheck run failed: %v", err) - return err - } - - lintResults, err := executor.Parse(log, output) - if err != nil { - log.Errorf("staticcheck parse output failed: %v", err) - return err - } - - if len(lintResults) == 0 { - return nil - } - - log.Infof("[%s] found total %d files with lint errors on repo %v", lintName, len(lintResults), orgRepo) - comments, err := gh.BuildPullRequestCommentBody(lintName, lintResults, a.PullRequestChangedFiles) - if err != nil { - log.Errorf("failed to build pull request comment body: %v", err) - return err - } - - if len(comments) == 0 { - // no related comments to post, continue to run other linters - return nil - } - - log.Infof("[%s] found valid %d comments related to this PR %d (%s) \n", lintName, len(comments), num, orgRepo) - if err := gh.PostPullReviewCommentsWithRetry(context.Background(), a.GithubClient, org, repo, num, comments); err != nil { - log.Errorf("failed to post comments: %v", err) - return err - } - return nil -} - -// Staticcheck is an executor that knows how to execute Staticcheck commands. -type Staticcheck struct { - // dir is the location of this repo. - dir string - // staticcheck is the path to the staticcheck binary. - staticcheck string - // execute executes a command - execute func(dir, command string, args ...string) ([]byte, error) -} - -// NewStaticcheckExecutor returns a new executor that knows how to execute staticcheck commands -// TODO: with config -func NewStaticcheckExecutor(dir string) (linters.Linter, error) { - log.Infof("staticcheck executor init") - g, err := exec.LookPath("staticcheck") - if err != nil { - return nil, err - } - return &Staticcheck{ - dir: dir, - staticcheck: g, - execute: func(dir, command string, args ...string) ([]byte, error) { - c := exec.Command(command, args...) - c.Dir = dir - log.Printf("final command: %v \n", c) - return c.Output() - }, - }, nil -} - -func (e *Staticcheck) Run(log *xlog.Logger, args ...string) ([]byte, error) { - b, err := e.execute(e.dir, e.staticcheck, args...) - if err != nil { - log.Errorf("staticcheck run with status: %v, mark and continue", err) - } else { - log.Infof("staticcheck succeeded") - } - - return b, nil -} - -// formatStaticcheckOutput formats the output of staticcheck -// example: -// gslb-api/utils.go:149:6: func dealWithRecordVipsId is unused (U1000) -// gslb-api/utils.go:162:2: this value of err is never used (SA4006) -// domain/repo/image.go:70:7: receiver name should be a reflection of its identity; don't use generic names such as "this" or "self" (ST1006) -// -// output: map[file][]linters.LinterOutput -func (e *Staticcheck) Parse(log *xlog.Logger, output []byte) (map[string][]linters.LinterOutput, error) { - return linters.FormatLinterOutput(log, output, linters.FormatLinterLine) + return linters.GeneralHandler(log, a, linters.GeneralParse) } diff --git a/internal/github/hunk.go b/internal/linters/hunk.go similarity index 99% rename from internal/github/hunk.go rename to internal/linters/hunk.go index 1eb7d706..5c5c5d32 100644 --- a/internal/github/hunk.go +++ b/internal/linters/hunk.go @@ -14,7 +14,7 @@ limitations under the License. */ -package github +package linters import ( "fmt" diff --git a/internal/github/hunk_test.go b/internal/linters/hunk_test.go similarity index 97% rename from internal/github/hunk_test.go rename to internal/linters/hunk_test.go index b8d06e26..de0caad3 100644 --- a/internal/github/hunk_test.go +++ b/internal/linters/hunk_test.go @@ -14,7 +14,7 @@ limitations under the License. */ -package github +package linters import ( "fmt" diff --git a/internal/linters/linters.go b/internal/linters/linters.go index dfbe6d73..5ec4f226 100644 --- a/internal/linters/linters.go +++ b/internal/linters/linters.go @@ -17,8 +17,17 @@ package linters import ( + "context" + "fmt" + "os/exec" + "path/filepath" + "regexp" + "strconv" + "strings" + "github.com/google/go-github/v57/github" "github.com/qiniu/reviewbot/config" + "github.com/qiniu/x/log" "github.com/qiniu/x/xlog" gitv2 "k8s.io/test-infra/prow/git/v2" ) @@ -94,3 +103,175 @@ If you have any questions about this comment, feel free to raise an issue here: - **https://github.com/qiniu/reviewbot** ` + +// ------------------------ Linter ------------------------ + +func GeneralHandler(log *xlog.Logger, a Agent, parse func(*xlog.Logger, []byte) (map[string][]LinterOutput, error)) error { + cmd := a.LinterConfig.Command + output, err := ExecRun(a.LinterConfig.WorkDir, cmd, a.LinterConfig.Args...) + if err != nil { + log.Errorf("%s run failed: %v, mark and continue", cmd, err) + } + + if len(output) == 0 { + return nil + } + + lintResults, err := parse(log, output) + if err != nil { + log.Errorf("failed to parse output failed: %v, cmd: %v", err, cmd) + return err + } + + if len(lintResults) == 0 { + return nil + } + + return Report(log, a, lintResults) + +} + +// ExecRun executes a command. +func ExecRun(workDir, command string, args ...string) ([]byte, error) { + g, err := exec.LookPath(command) + if err != nil { + return nil, err + } + + c := exec.Command(g, args...) + c.Dir = workDir + + return c.Output() +} + +// GeneralParse parses the output of a linter command. +func GeneralParse(log *xlog.Logger, output []byte) (map[string][]LinterOutput, error) { + return Parse(log, output, GeneralLineParser) +} + +// Report reports the lint results. +func Report(log *xlog.Logger, a Agent, lintResults map[string][]LinterOutput) error { + var ( + org = a.PullRequestEvent.Repo.GetOwner().GetLogin() + repo = a.PullRequestEvent.Repo.GetName() + num = a.PullRequestEvent.GetNumber() + orgRepo = a.PullRequestEvent.Repo.GetFullName() + linterName = a.LinterConfig.Command + ) + + log.Infof("[%s] found total %d files with lint errors on repo %v", linterName, len(lintResults), orgRepo) + comments, err := BuildPullRequestCommentBody(linterName, lintResults, a.PullRequestChangedFiles) + if err != nil { + log.Errorf("failed to build pull request comment body: %v", err) + return err + } + + if len(comments) == 0 { + // no related comments to post, continue to run other linters + return nil + } + + log.Infof("[%s] found valid %d comments related to this PR %d (%s) \n", linterName, len(comments), num, orgRepo) + if err := PostPullReviewCommentsWithRetry(context.Background(), a.GithubClient, org, repo, num, comments); err != nil { + log.Errorf("failed to post comments: %v", err) + return err + } + return nil +} + +// LineParser is a function that parses a line of linter output. +type LineParser func(line string) (*LinterOutput, error) + +// Parse parses the output of a linter command. +func Parse(log *xlog.Logger, output []byte, lineParser LineParser) (map[string][]LinterOutput, error) { + lines := strings.Split(string(output), "\n") + + var result = make(map[string][]LinterOutput) + for _, line := range lines { + if line == "" { + continue + } + output, err := lineParser(line) + if err != nil { + log.Warnf("unexpected linter check output: %v", line) + continue + } + + if output == nil { + continue + } + + if isGopAutoGeneratedFile(output.File) { + log.Infof("skip auto generated file by go+ : %s", output.File) + continue + } + + if outs, ok := result[output.File]; !ok { + result[output.File] = []LinterOutput{*output} + } else { + // remove duplicate + var existed bool + for _, v := range outs { + if v.File == output.File && v.Line == output.Line && v.Column == output.Column && v.Message == output.Message { + existed = true + break + } + } + + if !existed { + result[output.File] = append(result[output.File], *output) + } + } + } + + return result, nil +} + +// common format LinterLine +func GeneralLineParser(line string) (*LinterOutput, error) { + pattern := `^(.*):(\d+):(\d+): (.*)$` + regex, err := regexp.Compile(pattern) + if err != nil { + log.Errorf("compile regex failed: %v", err) + return nil, err + } + matches := regex.FindStringSubmatch(line) + if len(matches) != 5 { + return nil, fmt.Errorf("unexpected format, original: %s", line) + } + + lineNumber, err := strconv.ParseInt(matches[2], 10, 64) + if err != nil { + return nil, err + } + + columnNumber, err := strconv.ParseInt(matches[3], 10, 64) + if err != nil { + return nil, err + } + + return &LinterOutput{ + File: matches[1], + Line: int(lineNumber), + Column: int(columnNumber), + Message: matches[4], + }, nil +} + +var gop_auto_generated_file_pattern = `^.*_autogen.*.go$` +var gopGeneratedFileRegexp = regexp.MustCompile(gop_auto_generated_file_pattern) + +func isGopAutoGeneratedFile(file string) bool { + // TODO: need a more accurate way to determine whether it is a go+ auto generated file + return gopGeneratedFileRegexp.MatchString(filepath.Base(file)) +} + +func IsEmpty(args ...string) bool { + for _, arg := range args { + if arg != "" { + return false + } + } + + return true +} diff --git a/internal/linters/util_test.go b/internal/linters/linters_test.go similarity index 98% rename from internal/linters/util_test.go rename to internal/linters/linters_test.go index d9eeb9f7..73676495 100644 --- a/internal/linters/util_test.go +++ b/internal/linters/linters_test.go @@ -47,7 +47,7 @@ func TestFormatStaticcheckLine(t *testing.T) { } for _, c := range tc { - output, err := FormatLinterLine(c.input) + output, err := GeneralLineParser(c.input) if c.expected == nil && output != nil { t.Errorf("expected error, got: %v", output) } else { diff --git a/internal/linters/lua/luacheck/luacheck.go b/internal/linters/lua/luacheck/luacheck.go index 5c4cd730..06728108 100644 --- a/internal/linters/lua/luacheck/luacheck.go +++ b/internal/linters/lua/luacheck/luacheck.go @@ -1,119 +1,31 @@ package luacheck import ( - "bytes" - "context" - "os/exec" "strings" - gh "github.com/qiniu/reviewbot/internal/github" "github.com/qiniu/reviewbot/internal/linters" "github.com/qiniu/x/xlog" ) -var lintName = "luacheck" +// refer to https://github.com/mpeterv/luacheck +const linterName = "luacheck" func init() { - linters.RegisterPullRequestHandler(lintName, luaCheckHandler) + linters.RegisterPullRequestHandler(linterName, luacheckHandler) } -func luaCheckHandler(log *xlog.Logger, a linters.Agent) error { - var ( - org = a.PullRequestEvent.GetRepo().GetOwner().GetLogin() - repo = a.PullRequestEvent.GetRepo().GetName() - num = a.PullRequestEvent.GetNumber() - orgRepo = org + "/" + repo - ) - - executor, err := NewLuaCheckExecutor(a.LinterConfig.WorkDir) - if err != nil { - log.Errorf("init luacheck executor failed: %v", err) - return err - } - +func luacheckHandler(log *xlog.Logger, a linters.Agent) error { if linters.IsEmpty(a.LinterConfig.Args...) { a.LinterConfig.Args = append([]string{}, ".") } - output, err := executor.Run(log, a.LinterConfig.Args...) - if err != nil { - log.Errorf("luacheck run failed: %v", err) - return err - } - - lintResults, err := executor.Parse(log, output) - if err != nil { - log.Errorf("staticcheck parse output failed: %v", err) - return err - } - - if len(lintResults) == 0 { - return nil - } - - log.Infof("[%s] found total %d files with lint errors on repo %v", lintName, len(lintResults), orgRepo) - comments, err := gh.BuildPullRequestCommentBody(lintName, lintResults, a.PullRequestChangedFiles) - if err != nil { - log.Errorf("failed to build pull request comment body: %v", err) - return err - } - - if len(comments) == 0 { - // no related comments to post, continue to run other linters - return nil - } - - log.Infof("[%s] found valid %d comments related to this PR %d (%s) \n", lintName, len(comments), num, orgRepo) - if err := gh.PostPullReviewCommentsWithRetry(context.Background(), a.GithubClient, org, repo, num, comments); err != nil { - log.Errorf("failed to post comments: %v", err) - return err - } - return nil - -} - -// luacheck is an executor that knows how to execute luacheck commands. -type luacheck struct { - // dir is the location of this repo. - dir string - // luacheck is the path to the luacheck binary. - luacheck string -} - -func NewLuaCheckExecutor(dir string) (linters.Linter, error) { - g, err := exec.LookPath("luacheck") - if err != nil { - return nil, err - } - return &luacheck{ - dir: dir, - luacheck: g, - }, nil + return linters.GeneralHandler(log, a, func(l *xlog.Logger, output []byte) (map[string][]linters.LinterOutput, error) { + return linters.Parse(log, output, luacheckParser) + }) } -func (e *luacheck) Run(log *xlog.Logger, args ...string) ([]byte, error) { - c := exec.Command(e.luacheck, args...) - c.Dir = e.dir - var out bytes.Buffer - c.Stdout = &out - c.Stderr = &out - err := c.Run() - if err != nil { - log.Errorf("luacheck run with status: %v, mark and continue", err) - } else { - log.Infof("luacheck succeeded") - } - - return out.Bytes(), nil -} - -func (e *luacheck) Parse(log *xlog.Logger, output []byte) (map[string][]linters.LinterOutput, error) { - return linters.FormatLinterOutput(log, output, formatLuaCheckLine) -} - -func formatLuaCheckLine(line string) (*linters.LinterOutput, error) { - - lineResult, err := linters.FormatLinterLine(line) +func luacheckParser(line string) (*linters.LinterOutput, error) { + lineResult, err := linters.GeneralLineParser(line) if err != nil { return nil, err diff --git a/internal/linters/lua/luacheck/luacheck_test.go b/internal/linters/lua/luacheck/luacheck_test.go index 7afa1fbd..c703cb36 100644 --- a/internal/linters/lua/luacheck/luacheck_test.go +++ b/internal/linters/lua/luacheck/luacheck_test.go @@ -55,7 +55,7 @@ func TestFormatLuaCheckLine(t *testing.T) { } for _, c := range tc { - output, err := formatLuaCheckLine(c.input) + output, err := luacheckParser(c.input) if c.expected == nil && output != nil { t.Errorf("expected error, got: %v", output) } else { diff --git a/internal/linters/util.go b/internal/linters/util.go deleted file mode 100644 index 13c26e6b..00000000 --- a/internal/linters/util.go +++ /dev/null @@ -1,107 +0,0 @@ -package linters - -import ( - "fmt" - "path/filepath" - "regexp" - "strconv" - "strings" - - "github.com/qiniu/x/log" - "github.com/qiniu/x/xlog" -) - -func IsEmpty(args ...string) bool { - for _, arg := range args { - if arg != "" { - return false - } - } - - return true -} - -type FormatLinterLineFunc func(line string) (*LinterOutput, error) - -func FormatLinterOutput(log *xlog.Logger, output []byte, FormatLinter FormatLinterLineFunc) (map[string][]LinterOutput, error) { - lines := strings.Split(string(output), "\n") - - var result = make(map[string][]LinterOutput) - for _, line := range lines { - if line == "" { - continue - } - output, err := FormatLinter(line) - if err != nil { - log.Warnf("unexpected linter check output: %v", line) - continue - } - - if output == nil { - continue - } - - if isGopAutoGeneratedFile(output.File) { - log.Infof("skip auto generated file by go+ : %s", output.File) - continue - } - - if outs, ok := result[output.File]; !ok { - result[output.File] = []LinterOutput{*output} - } else { - // remove duplicate - var existed bool - for _, v := range outs { - if v.File == output.File && v.Line == output.Line && v.Column == output.Column && v.Message == output.Message { - existed = true - break - } - } - - if !existed { - result[output.File] = append(result[output.File], *output) - } - } - } - - return result, nil -} - -// common format LinterLine -func FormatLinterLine(line string) (*LinterOutput, error) { - pattern := `^(.*):(\d+):(\d+): (.*)$` - regex, err := regexp.Compile(pattern) - if err != nil { - log.Errorf("compile regex failed: %v", err) - return nil, err - } - matches := regex.FindStringSubmatch(line) - if len(matches) != 5 { - return nil, fmt.Errorf("unexpected format, original: %s", line) - } - - lineNumber, err := strconv.ParseInt(matches[2], 10, 64) - if err != nil { - return nil, err - } - - columnNumber, err := strconv.ParseInt(matches[3], 10, 64) - if err != nil { - return nil, err - } - - return &LinterOutput{ - File: matches[1], - Line: int(lineNumber), - Column: int(columnNumber), - Message: matches[4], - }, nil -} - -var gop_auto_generated_file_pattern = `^.*_autogen.*.go$` -var gopGeneratedFileRegexp = regexp.MustCompile(gop_auto_generated_file_pattern) - -func isGopAutoGeneratedFile(file string) bool { - // TODO: need a more accurate way to determine whether it is a go+ auto generated file - return gopGeneratedFileRegexp.MatchString(filepath.Base(file)) -} diff --git a/server.go b/server.go index 9eb89410..261f0fcb 100644 --- a/server.go +++ b/server.go @@ -25,7 +25,6 @@ import ( "github.com/google/go-github/v57/github" "github.com/gregjones/httpcache" "github.com/qiniu/reviewbot/config" - gh "github.com/qiniu/reviewbot/internal/github" "github.com/qiniu/reviewbot/internal/linters" "github.com/qiniu/x/log" "github.com/qiniu/x/xlog" @@ -97,7 +96,7 @@ func (s *Server) handle(log *xlog.Logger, ctx context.Context, event *github.Pul installationID := event.GetInstallation().GetID() log.Infof("processing pull request %d, (%v/%v), installationID: %d\n", num, org, repo, installationID) - pullRequestAffectedFiles, response, err := gh.ListPullRequestsFiles(ctx, s.GithubClient(installationID), org, repo, num) + pullRequestAffectedFiles, response, err := linters.ListPullRequestsFiles(ctx, s.GithubClient(installationID), org, repo, num) if err != nil { return err } @@ -140,7 +139,9 @@ func (s *Server) handle(log *xlog.Logger, ctx context.Context, event *github.Pul lingerConfig.WorkDir = r.Directory() } - log.Infof("[%s] config on repo %v: %v", name, orgRepo, lingerConfig) + lingerConfig = config.FixLinterConfig(lingerConfig, name) + + log.Infof("[%s] config on repo %v: %+v", name, orgRepo, lingerConfig) agent := linters.Agent{ GithubClient: s.GithubClient(installationID),