diff --git a/doghouse/server/doghouse.go b/doghouse/server/doghouse.go index 450e3b83ea..7be8abaeef 100644 --- a/doghouse/server/doghouse.go +++ b/doghouse/server/doghouse.go @@ -12,6 +12,7 @@ import ( "github.com/reviewdog/reviewdog" "github.com/reviewdog/reviewdog/diff" "github.com/reviewdog/reviewdog/doghouse" + "github.com/reviewdog/reviewdog/service/github/githubutils" ) // GitHub check runs API cannot handle too large requests. @@ -188,28 +189,13 @@ func (ch *Checker) summaryFindings(name string, checks []*reviewdog.FilteredChec lines = append(lines, "... (Too many findings. Dropped some findings)") break } - lines = append(lines, ch.buildFindingLink(c)) + lines = append(lines, githubutils.LinkedMarkdownCheckResult( + ch.req.Owner, ch.req.Repo, ch.req.SHA, c.CheckResult)) } lines = append(lines, "") return lines } -func (ch *Checker) buildFindingLink(c *reviewdog.FilteredCheck) string { - if c.Path == "" { - return c.Message - } - loc := c.Path - link := ch.brobHRef(c.Path) - if c.Lnum != 0 { - loc = fmt.Sprintf("%s:%d", loc, c.Lnum) - link = fmt.Sprintf("%s#L%d", link, c.Lnum) - } - if c.Col != 0 { - loc = fmt.Sprintf("%s:%d", loc, c.Col) - } - return fmt.Sprintf("[%s](%s): %s", loc, link, c.Message) -} - func (ch *Checker) toCheckRunAnnotation(c *reviewdog.FilteredCheck) *github.CheckRunAnnotation { a := &github.CheckRunAnnotation{ Path: github.String(c.Path), @@ -227,10 +213,6 @@ func (ch *Checker) toCheckRunAnnotation(c *reviewdog.FilteredCheck) *github.Chec return a } -func (ch *Checker) brobHRef(path string) string { - return fmt.Sprintf("http://github.com/%s/%s/blob/%s/%s", ch.req.Owner, ch.req.Repo, ch.req.SHA, path) -} - func (ch *Checker) diff(ctx context.Context) ([]*diff.FileDiff, error) { d, err := ch.rawDiff(ctx) if err != nil { diff --git a/service/github/github.go b/service/github/github.go index 2c0463e2aa..a9440429dc 100644 --- a/service/github/github.go +++ b/service/github/github.go @@ -4,16 +4,20 @@ import ( "context" "fmt" "path/filepath" + "strings" "sync" "github.com/google/go-github/v28/github" "github.com/reviewdog/reviewdog" + "github.com/reviewdog/reviewdog/service/github/githubutils" "github.com/reviewdog/reviewdog/service/serviceutil" ) var _ reviewdog.CommentService = &GitHubPullRequest{} var _ reviewdog.DiffService = &GitHubPullRequest{} +const maxCommentsPerRequest = 30 + // GitHubPullRequest is a comment and diff service for GitHub PullRequest. // // API: @@ -75,10 +79,23 @@ func (g *GitHubPullRequest) Flush(ctx context.Context) error { func (g *GitHubPullRequest) postAsReviewComment(ctx context.Context) error { comments := make([]*github.DraftReviewComment, 0, len(g.postComments)) + remaining := make([]*reviewdog.Comment, 0) for _, c := range g.postComments { if g.postedcs.IsPosted(c, c.LnumDiff) { continue } + // Only posts maxCommentsPerRequest comments per 1 request to avoid spammy + // review comments. An example GitHub error if we don't limit the # of + // review comments. + // + // > 403 You have triggered an abuse detection mechanism and have been + // > temporarily blocked from content creation. Please retry your request + // > again later. + // https://developer.github.com/v3/#abuse-rate-limits + if len(comments) >= maxCommentsPerRequest { + remaining = append(remaining, c) + continue + } cbody := serviceutil.CommentBody(c) comments = append(comments, &github.DraftReviewComment{ Path: &c.Path, @@ -97,11 +114,36 @@ func (g *GitHubPullRequest) postAsReviewComment(ctx context.Context) error { CommitID: &g.sha, Event: github.String("COMMENT"), Comments: comments, + Body: github.String(g.remainingCommentsSummary(remaining)), } _, _, err := g.cli.PullRequests.CreateReview(ctx, g.owner, g.repo, g.pr, review) return err } +func (g *GitHubPullRequest) remainingCommentsSummary(remaining []*reviewdog.Comment) string { + if len(remaining) == 0 { + return "" + } + perTool := make(map[string][]*reviewdog.Comment) + for _, c := range remaining { + perTool[c.ToolName] = append(perTool[c.ToolName], c) + } + var sb strings.Builder + sb.WriteString("Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit\n") + sb.WriteString("\n") + for tool, comments := range perTool { + sb.WriteString("
\n") + sb.WriteString(fmt.Sprintf("%s\n", tool)) + sb.WriteString("\n") + for _, c := range comments { + sb.WriteString(githubutils.LinkedMarkdownCheckResult(g.owner, g.repo, g.sha, c.CheckResult)) + sb.WriteString("\n") + } + sb.WriteString("
\n") + } + return sb.String() +} + func (g *GitHubPullRequest) setPostedComment(ctx context.Context) error { g.postedcs = make(serviceutil.PostedComments) cs, err := g.comment(ctx) diff --git a/service/github/github_test.go b/service/github/github_test.go index 6a282e6b08..2ac685fc85 100644 --- a/service/github/github_test.go +++ b/service/github/github_test.go @@ -221,7 +221,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { if *req.Event != "COMMENT" { t.Errorf("PullRequestReviewRequest.Event = %v, want COMMENT", *req.Event) } - if req.Body != nil { + if req.Body != nil && *req.Body != "" { t.Errorf("PullRequestReviewRequest.Body = %v, want empty", *req.Body) } if *req.CommitID != "sha" { @@ -286,6 +286,69 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) { } } +func TestGitHubPullRequest_Post_toomany(t *testing.T) { + cwd, _ := os.Getwd() + defer os.Chdir(cwd) + + moveToRootDir() + + listCommentsAPICalled := 0 + postCommentsAPICalled := 0 + + mux := http.NewServeMux() + mux.HandleFunc("/repos/o/r/pulls/14/comments", func(w http.ResponseWriter, r *http.Request) { + listCommentsAPICalled++ + if err := json.NewEncoder(w).Encode([]*github.PullRequestComment{}); err != nil { + t.Fatal(err) + } + }) + mux.HandleFunc("/repos/o/r/pulls/14/reviews", func(w http.ResponseWriter, r *http.Request) { + postCommentsAPICalled++ + var req github.PullRequestReviewRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + t.Error(err) + } + if req.GetBody() == "" { + t.Errorf("PullRequestReviewRequest.Body is empty but want some summary text") + } + }) + ts := httptest.NewServer(mux) + defer ts.Close() + + cli := github.NewClient(nil) + cli.BaseURL, _ = url.Parse(ts.URL + "/") + g, err := NewGitHubPullRequest(cli, "o", "r", 14, "sha") + if err != nil { + t.Fatal(err) + } + comments := []*reviewdog.Comment{} + for i := 0; i < 100; i++ { + comments = append(comments, &reviewdog.Comment{ + CheckResult: &reviewdog.CheckResult{ + Path: "reviewdog.go", + Lnum: i, + }, + LnumDiff: i, + Body: "comment", + ToolName: "tool", + }) + } + for _, c := range comments { + if err := g.Post(context.Background(), c); err != nil { + t.Error(err) + } + } + if err := g.Flush(context.Background()); err != nil { + t.Error(err) + } + if want := 1; listCommentsAPICalled != want { + t.Errorf("GitHub List PullRequest comments API called %v times, want %d times", listCommentsAPICalled, want) + } + if want := 1; postCommentsAPICalled != want { + t.Errorf("GitHub post PullRequest comments API called %v times, want %d times", postCommentsAPICalled, want) + } +} + func TestGitHubPullRequest_workdir(t *testing.T) { cwd, _ := os.Getwd() defer os.Chdir(cwd) diff --git a/service/github/githubutils/utils.go b/service/github/githubutils/utils.go new file mode 100644 index 0000000000..48072e858e --- /dev/null +++ b/service/github/githubutils/utils.go @@ -0,0 +1,43 @@ +package githubutils + +import ( + "fmt" + + "github.com/reviewdog/reviewdog" +) + +// LinkedMarkdownCheckResult returns Markdown string which contains a link to the +// location in the CheckResult and CheckResult content itself. +func LinkedMarkdownCheckResult(owner, repo, sha string, c *reviewdog.CheckResult) string { + if c.Path == "" { + return c.Message + } + loc := BasicLocationFormat(c) + link := PathLink(owner, repo, sha, c.Path, c.Lnum) + return fmt.Sprintf("[%s](%s) %s", loc, link, c.Message) +} + +// PathLink build a link to GitHub path to given sha, file, and line. +func PathLink(owner, repo, sha, path string, line int) string { + if sha == "" { + sha = "master" + } + fragment := "" + if line > 0 { + fragment = fmt.Sprintf("#L%d", line) + } + return fmt.Sprintf("http://github.com/%s/%s/blob/%s/%s%s", + owner, repo, sha, path, fragment) +} + +// BasicLocationFormat format chec CheckResult to %f|%l col %c| erorformat. +func BasicLocationFormat(c *reviewdog.CheckResult) string { + loc := c.Path + "|" + if c.Lnum != 0 { + loc = fmt.Sprintf("%s%d", loc, c.Lnum) + if c.Col != 0 { + loc = fmt.Sprintf("%s col %d", loc, c.Col) + } + } + return loc + "|" +} diff --git a/service/github/githubutils/utils_test.go b/service/github/githubutils/utils_test.go new file mode 100644 index 0000000000..ffc6612c74 --- /dev/null +++ b/service/github/githubutils/utils_test.go @@ -0,0 +1,58 @@ +package githubutils + +import ( + "testing" + + "github.com/reviewdog/reviewdog" +) + +func TestLinkedMarkdownCheckResult(t *testing.T) { + tests := []struct { + owner, repo, sha string + c *reviewdog.CheckResult + want string + }{ + { + owner: "o", + repo: "r", + sha: "s", + c: &reviewdog.CheckResult{ + Path: "path/to/file.txt", + Lnum: 1414, + Col: 14, + Message: "msg", + }, + want: "[path/to/file.txt|1414 col 14|](http://github.com/o/r/blob/s/path/to/file.txt#L1414) msg", + }, + { + owner: "o", + repo: "r", + sha: "s", + c: &reviewdog.CheckResult{ + Path: "path/to/file.txt", + Lnum: 1414, + Col: 0, + Message: "msg", + }, + want: "[path/to/file.txt|1414|](http://github.com/o/r/blob/s/path/to/file.txt#L1414) msg", + }, + { + owner: "o", + repo: "r", + sha: "s", + c: &reviewdog.CheckResult{ + Path: "path/to/file.txt", + Lnum: 0, + Col: 0, + Message: "msg", + }, + want: "[path/to/file.txt||](http://github.com/o/r/blob/s/path/to/file.txt) msg", + }, + } + for _, tt := range tests { + if got := LinkedMarkdownCheckResult(tt.owner, tt.repo, tt.sha, tt.c); got != tt.want { + t.Errorf("LinkedMarkdownCheckResult(%q, %q, %q, %#v) = %q, want %q", + tt.owner, tt.repo, tt.sha, tt.c, got, tt.want) + } + } +}