From e2f787f32807c746eedaf421ff89409882575153 Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Thu, 18 Jul 2024 14:09:11 +0100 Subject: [PATCH] Refactor github reporer to use common comments code --- cmd/pint/ci.go | 12 +- internal/reporter/comments.go | 134 ++++++++++++++++ internal/reporter/comments_test.go | 176 ++++++++++++++++++++ internal/reporter/github.go | 247 ++++++++++++++--------------- internal/reporter/github_test.go | 206 ++++++++++++++---------- internal/reporter/gitlab.go | 26 +-- internal/reporter/gitlab_test.go | 85 +--------- internal/reporter/reporter.go | 106 ------------- 8 files changed, 575 insertions(+), 417 deletions(-) diff --git a/cmd/pint/ci.go b/cmd/pint/ci.go index f0704765..dd579af8 100644 --- a/cmd/pint/ci.go +++ b/cmd/pint/ci.go @@ -171,7 +171,7 @@ func actionCI(c *cli.Context) error { ); err != nil { return err } - reps = append(reps, gl) + reps = append(reps, reporter.NewCommentReporter(gl)) } meta.cfg.Repository = detectRepository(meta.cfg.Repository) @@ -191,6 +191,12 @@ func actionCI(c *cli.Context) error { return fmt.Errorf("got not a valid number via GITHUB_PULL_REQUEST_NUMBER: %w", err) } + var headCommit string + headCommit, err = git.HeadCommit(git.RunGit) + if err != nil { + return errors.New("failed to get the HEAD commit") + } + timeout, _ := time.ParseDuration(meta.cfg.Repository.GitHub.Timeout) var gr reporter.GithubReporter if gr, err = reporter.NewGithubReporter( @@ -203,11 +209,11 @@ func actionCI(c *cli.Context) error { meta.cfg.Repository.GitHub.Repo, prNum, meta.cfg.Repository.GitHub.MaxComments, - git.RunGit, + headCommit, ); err != nil { return err } - reps = append(reps, gr) + reps = append(reps, reporter.NewCommentReporter(gr)) } minSeverity, err := checks.ParseSeverity(c.String(failOnFlag)) diff --git a/internal/reporter/comments.go b/internal/reporter/comments.go index ca0237c2..b5cfaa2a 100644 --- a/internal/reporter/comments.go +++ b/internal/reporter/comments.go @@ -2,10 +2,12 @@ package reporter import ( "context" + "log/slog" "slices" "strings" "github.com/cloudflare/pint/internal/checks" + "github.com/cloudflare/pint/internal/output" ) type PendingCommentV2 struct { @@ -30,9 +32,22 @@ type Commenter interface { Create(context.Context, any, PendingCommentV2) error Delete(context.Context, any, ExistingCommentV2) error CanCreate(int) (bool, error) + CanDelete(ExistingCommentV2) bool IsEqual(ExistingCommentV2, PendingCommentV2) bool } +func NewCommentReporter(c Commenter) CommentReporter { + return CommentReporter{c: c} +} + +type CommentReporter struct { + c Commenter +} + +func (cr CommentReporter) Submit(summary Summary) (err error) { + return Submit(context.Background(), summary, cr.c) +} + func makeComments(summary Summary) (comments []PendingCommentV2) { var buf strings.Builder for _, reports := range dedupReports(summary.reports) { @@ -155,3 +170,122 @@ func problemIcon(s checks.Severity) string { return ":stop_sign:" } } + +func errsToComment(errs []error) string { + var buf strings.Builder + buf.WriteString("There were some errors when pint was trying to create a report.\n") + buf.WriteString("Some review comments might be outdated or missing.\n") + buf.WriteString("List of all errors:\n\n") + for _, err := range errs { + buf.WriteString("- `") + buf.WriteString(err.Error()) + buf.WriteString("`\n") + } + return buf.String() +} + +func Submit(ctx context.Context, s Summary, c Commenter) error { + dsts, err := c.Destinations(ctx) + if err != nil { + return err + } + + for _, dst := range dsts { + if err = updateDestination(ctx, s, c, dst); err != nil { + return err + } + } + + return nil +} + +func updateDestination(ctx context.Context, s Summary, c Commenter, dst any) (err error) { + slog.Info("Listing existing comments", slog.String("reporter", c.Describe())) + existingComments, err := c.List(ctx, dst) + if err != nil { + return err + } + + var created int + var ok bool + var errs []error + pendingComments := makeComments(s) + for _, pending := range pendingComments { + for _, existing := range existingComments { + if c.IsEqual(existing, pending) { + slog.Debug("Comment already exists", + slog.String("reporter", c.Describe()), + slog.String("path", pending.path), + slog.Int("line", pending.line), + ) + goto NEXTCreate + } + } + slog.Debug("Comment doesn't exist yet", + slog.String("reporter", c.Describe()), + slog.String("path", pending.path), + slog.Int("line", pending.line), + ) + + ok, err = c.CanCreate(created) + if err != nil { + errs = append(errs, err) + } + if !ok { + slog.Debug("Cannot create new comment", + slog.String("reporter", c.Describe()), + slog.String("path", pending.path), + slog.Int("line", pending.line), + ) + goto NEXTCreate + } + + if err := c.Create(ctx, dst, pending); err != nil { + slog.Error("Failed to create a new comment", + slog.String("reporter", c.Describe()), + slog.String("path", pending.path), + slog.Int("line", pending.line), + slog.Any("err", err), + ) + return err + } + created++ + NEXTCreate: + } + + for _, existing := range existingComments { + for _, pending := range pendingComments { + if c.IsEqual(existing, pending) { + goto NEXTDelete + } + } + if !c.CanDelete(existing) { + goto NEXTDelete + } + if err := c.Delete(ctx, dst, existing); err != nil { + slog.Error("Failed to delete a stale comment", + slog.String("reporter", c.Describe()), + slog.String("path", existing.path), + slog.Int("line", existing.line), + slog.Any("err", err), + ) + errs = append(errs, err) + } + NEXTDelete: + } + + slog.Info("Creating report summary", + slog.String("reporter", c.Describe()), + slog.Int("reports", len(s.reports)), + slog.Int("online", int(s.OnlineChecks)), + slog.Int("offline", int(s.OnlineChecks)), + slog.String("duration", output.HumanizeDuration(s.Duration)), + slog.Int("entries", s.TotalEntries), + slog.Int("checked", int(s.CheckedEntries)), + ) + if err := c.Summary(ctx, dst, s, errs); err != nil { + return err + } + + return nil +} diff --git a/internal/reporter/comments_test.go b/internal/reporter/comments_test.go index 48e34f9a..bcfeacec 100644 --- a/internal/reporter/comments_test.go +++ b/internal/reporter/comments_test.go @@ -5,7 +5,11 @@ import ( "errors" "fmt" "log/slog" + "net/http" + "net/http/httptest" + "strings" "testing" + "time" "github.com/neilotoole/slogt" "github.com/stretchr/testify/require" @@ -57,6 +61,10 @@ func (tc testCommenter) Delete(ctx context.Context, dst any, comment ExistingCom return tc.delete(ctx, dst, comment) } +func (tc testCommenter) CanDelete(ExistingCommentV2) bool { + return true +} + func (tc testCommenter) CanCreate(n int) (bool, error) { return tc.canCreate(n) } @@ -769,3 +777,171 @@ foo details }) } } + +func TestCommentsCommonPaths(t *testing.T) { + type errorCheck func(err error) error + + type testCaseT struct { + httpHandler http.Handler + errorHandler errorCheck + + description string + branch string + token string + + reports []Report + timeout time.Duration + project int + maxComments int + } + + p := parser.NewParser(false) + mockRules, _ := p.Parse([]byte(` +- record: target is down + expr: up == 0 +- record: sum errors + expr: sum(errors) by (job) +`)) + + fooReport := Report{ + Path: discovery.Path{ + SymlinkTarget: "foo.txt", + Name: "foo.txt", + }, + ModifiedLines: []int{2}, + Rule: mockRules[0], + Problem: checks.Problem{ + Reporter: "foo", + Text: "foo error", + Details: "foo details", + Lines: parser.LineRange{First: 1, Last: 3}, + Severity: checks.Fatal, + Anchor: checks.AnchorAfter, + }, + } + + testCases := []testCaseT{ + { + description: "returns an error on non-200 HTTP response", + branch: "fakeBranch", + token: "fakeToken", + timeout: time.Second, + project: 123, + maxComments: 50, + reports: []Report{fooReport}, + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(400) + _, _ = w.Write([]byte("Bad Request")) + }), + errorHandler: func(err error) error { + if err != nil { + return nil + } + return fmt.Errorf("wrong error: %w", err) + }, + }, + { + description: "returns an error on HTTP response timeout", + branch: "fakeBranch", + token: "fakeToken", + timeout: time.Second, + project: 123, + maxComments: 50, + reports: []Report{fooReport}, + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + time.Sleep(time.Second * 2) + w.WriteHeader(400) + _, _ = w.Write([]byte("Bad Request")) + }), + errorHandler: func(err error) error { + if err != nil && strings.HasSuffix(err.Error(), "context deadline exceeded") { + return nil + } + return fmt.Errorf("wrong error: %w", err) + }, + }, + { + description: "returns an error on non-json body", + branch: "fakeBranch", + token: "fakeToken", + timeout: time.Second, + project: 123, + maxComments: 50, + reports: []Report{fooReport}, + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(200) + _, _ = w.Write([]byte("OK")) + }), + errorHandler: func(err error) error { + if err != nil { + return nil + } + return fmt.Errorf("wrong error: %w", err) + }, + }, + { + description: "returns an error on empty JSON body", + branch: "fakeBranch", + token: "fakeToken", + timeout: time.Second, + project: 123, + maxComments: 50, + reports: []Report{fooReport}, + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(200) + _, _ = w.Write([]byte("{}")) + }), + errorHandler: func(err error) error { + if err != nil { + return nil + } + return fmt.Errorf("wrong error: %w", err) + }, + }, + } + + for _, tc := range testCases { + for _, c := range []func(string) Commenter{ + func(uri string) Commenter { + r, err := NewGitLabReporter( + "v0.0.0", + tc.branch, + uri, + tc.timeout, + tc.token, + tc.project, + tc.maxComments, + ) + require.NoError(t, err, "can't create gitlab reporter") + return r + }, + func(uri string) Commenter { + r, err := NewGithubReporter( + "v0.0.0", + uri, + uri, + tc.timeout, + tc.token, + "owner", + "repo", + 123, + tc.maxComments, + "fake-commit-id", + ) + require.NoError(t, err, "can't create gitlab reporter") + return r + }, + } { + t.Run(tc.description, func(t *testing.T) { + slog.SetDefault(slogt.New(t)) + + srv := httptest.NewServer(tc.httpHandler) + defer srv.Close() + + summary := NewSummary(tc.reports) + err := Submit(context.Background(), summary, c(srv.URL)) + require.NoError(t, tc.errorHandler(err)) + }) + } + } +} diff --git a/internal/reporter/github.go b/internal/reporter/github.go index 2ea62a70..fb5837ea 100644 --- a/internal/reporter/github.go +++ b/internal/reporter/github.go @@ -3,6 +3,7 @@ package reporter import ( "bytes" "context" + "errors" "fmt" "log/slog" "strconv" @@ -13,14 +14,13 @@ import ( "golang.org/x/oauth2" "github.com/cloudflare/pint/internal/checks" - "github.com/cloudflare/pint/internal/git" "github.com/cloudflare/pint/internal/output" ) var reviewBody = "### This pull request was validated by [pint](https://github.com/cloudflare/pint).\n" type GithubReporter struct { - gitCmd git.CommandRunner + headCommit string client *github.Client version string @@ -34,9 +34,13 @@ type GithubReporter struct { maxComments int } +type ghCommentMeta struct { + id int64 +} + // NewGithubReporter creates a new GitHub reporter that reports // problems via comments on a given pull request number (integer). -func NewGithubReporter(version, baseURL, uploadURL string, timeout time.Duration, token, owner, repo string, prNum, maxComments int, gitCmd git.CommandRunner) (_ GithubReporter, err error) { +func NewGithubReporter(version, baseURL, uploadURL string, timeout time.Duration, token, owner, repo string, prNum, maxComments int, headCommit string) (_ GithubReporter, err error) { slog.Info( "Will report problems to GitHub", slog.String("baseURL", baseURL), @@ -46,6 +50,7 @@ func NewGithubReporter(version, baseURL, uploadURL string, timeout time.Duration slog.String("repo", repo), slog.Int("pr", prNum), slog.Int("maxComments", maxComments), + slog.String("headCommit", headCommit), ) gr := GithubReporter{ version: version, @@ -57,7 +62,7 @@ func NewGithubReporter(version, baseURL, uploadURL string, timeout time.Duration repo: repo, prNum: prNum, maxComments: maxComments, - gitCmd: gitCmd, + headCommit: headCommit, } ts := oauth2.StaticTokenSource( @@ -76,141 +81,172 @@ func NewGithubReporter(version, baseURL, uploadURL string, timeout time.Duration return gr, nil } -// Submit submits the summary to GitHub. -func (gr GithubReporter) Submit(summary Summary) error { - headCommit, err := git.HeadCommit(gr.gitCmd) - if err != nil { - return fmt.Errorf("failed to get HEAD commit: %w", err) - } - slog.Info("Got HEAD commit from git", slog.String("commit", headCommit)) +func (gr GithubReporter) Describe() string { + return "GitHub" +} - review, err := gr.findExistingReview() +func (gr GithubReporter) Destinations(context.Context) ([]any, error) { + return []any{gr.prNum}, nil +} + +func (gr GithubReporter) Summary(ctx context.Context, _ any, summary Summary, errs []error) error { + review, err := gr.findExistingReview(ctx) if err != nil { return fmt.Errorf("failed to list pull request reviews: %w", err) } if review != nil { - if err = gr.updateReview(review, summary); err != nil { + if err = gr.updateReview(ctx, review, summary); err != nil { return fmt.Errorf("failed to update pull request review: %w", err) } } else { - if err = gr.createReview(headCommit, summary); err != nil { + if err = gr.createReview(ctx, summary); err != nil { return fmt.Errorf("failed to create pull request review: %w", err) } } - return gr.addReviewComments(headCommit, summary) + if len(errs) > 0 { + if err = gr.generalComment(ctx, errsToComment(errs)); err != nil { + return err + } + } + + return nil } -func (gr GithubReporter) findExistingReview() (*github.PullRequestReview, error) { - ctx, cancel := context.WithTimeout(context.Background(), gr.timeout) +func (gr GithubReporter) List(ctx context.Context, _ any) ([]ExistingCommentV2, error) { + reqCtx, cancel := context.WithTimeout(ctx, gr.timeout) defer cancel() - reviews, _, err := gr.client.PullRequests.ListReviews(ctx, gr.owner, gr.repo, gr.prNum, nil) + slog.Debug("Getting the list of pull request comments", slog.Int("pr", gr.prNum)) + existing, _, err := gr.client.PullRequests.ListComments(reqCtx, gr.owner, gr.repo, gr.prNum, nil) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to list pull request reviews: %w", err) } - for _, review := range reviews { - if strings.HasPrefix(review.GetBody(), reviewBody) { - return review, nil + comments := make([]ExistingCommentV2, 0, len(existing)) + for _, ec := range existing { + if ec.GetPath() == "" { + slog.Debug("Skipping general comment", slog.Int64("id", ec.GetID())) + continue } + comments = append(comments, ExistingCommentV2{ + path: ec.GetPath(), + text: ec.GetBody(), + line: ec.GetLine(), + meta: ghCommentMeta{id: ec.GetID()}, + }) } - return nil, nil + return comments, nil } -func (gr GithubReporter) updateReview(review *github.PullRequestReview, summary Summary) error { - slog.Info("Updating pull request review", slog.String("repo", fmt.Sprintf("%s/%s", gr.owner, gr.repo))) +func (gr GithubReporter) Create(ctx context.Context, _ any, p PendingCommentV2) error { + var side string + if p.anchor == checks.AnchorBefore { + side = "LEFT" + } else { + side = "RIGHT" + } + + comment := &github.PullRequestComment{ + CommitID: github.String(gr.headCommit), + Path: github.String(p.path), + Body: github.String(p.text), + Line: github.Int(p.line), + Side: github.String(side), + } + + slog.Debug("Creating a review comment", slog.String("body", comment.GetBody()), slog.String("commit", comment.GetCommitID())) - ctx, cancel := context.WithTimeout(context.Background(), gr.timeout) + reqCtx, cancel := context.WithTimeout(ctx, gr.timeout) defer cancel() - _, _, err := gr.client.PullRequests.UpdateReview( - ctx, - gr.owner, - gr.repo, - gr.prNum, - review.GetID(), - formatGHReviewBody(gr.version, summary), - ) + _, _, err := gr.client.PullRequests.CreateComment(reqCtx, gr.owner, gr.repo, gr.prNum, comment) return err } -func (gr GithubReporter) addReviewComments(headCommit string, summary Summary) error { - slog.Info("Creating review comments") +func (gr GithubReporter) Delete(ctx context.Context, _ any, comment ExistingCommentV2) error { + meta := comment.meta.(ghCommentMeta) + slog.Debug("Deleting stale pull request request comment", + slog.Int64("id", meta.id), + ) - existingComments, err := gr.getReviewComments() - if err != nil { - return err - } + reqCtx, cancel := context.WithTimeout(ctx, gr.timeout) + defer cancel() - var added int - for _, rep := range summary.Reports() { - comment := reportToGitHubComment(headCommit, rep) - - var found bool - for _, ec := range existingComments { - if ec.GetBody() == comment.GetBody() && - ec.GetCommitID() == comment.GetCommitID() && - ec.GetLine() == comment.GetLine() { - found = true - break - } - } - if found { - slog.Debug("Comment already exist", - slog.String("path", comment.GetPath()), - slog.Int("line", comment.GetLine()), - slog.String("commit", comment.GetCommitID()), - slog.String("body", comment.GetBody()), - ) - continue - } + _, err := gr.client.PullRequests.DeleteComment(reqCtx, gr.owner, gr.repo, meta.id) + return err +} - if err := gr.createComment(comment); err != nil { - return err - } - added++ +func (gr GithubReporter) CanDelete(ExistingCommentV2) bool { + return false +} - if added >= gr.maxComments { - return gr.tooManyComments(len(summary.Reports())) - } +func (gr GithubReporter) CanCreate(done int) (bool, error) { + if done >= gr.maxComments { + return false, errors.New(tooManyCommentsMsg(done, gr.maxComments)) } + return true, nil +} - return nil +func (gr GithubReporter) IsEqual(existing ExistingCommentV2, pending PendingCommentV2) bool { + if existing.path != pending.path { + return false + } + if existing.line != pending.line { + return false + } + return strings.Trim(existing.text, "\n") == strings.Trim(pending.text, "\n") } -func (gr GithubReporter) getReviewComments() ([]*github.PullRequestComment, error) { - ctx, cancel := context.WithTimeout(context.Background(), gr.timeout) +func (gr GithubReporter) findExistingReview(ctx context.Context) (*github.PullRequestReview, error) { + reqCtx, cancel := context.WithTimeout(ctx, gr.timeout) defer cancel() - comments, _, err := gr.client.PullRequests.ListComments(ctx, gr.owner, gr.repo, gr.prNum, nil) - return comments, err + reviews, _, err := gr.client.PullRequests.ListReviews(reqCtx, gr.owner, gr.repo, gr.prNum, nil) + if err != nil { + return nil, err + } + + for _, review := range reviews { + if strings.HasPrefix(review.GetBody(), reviewBody) { + return review, nil + } + } + + return nil, nil } -func (gr GithubReporter) createComment(comment *github.PullRequestComment) error { - slog.Debug("Creating review comment", slog.String("body", comment.GetBody()), slog.String("commit", comment.GetCommitID())) +func (gr GithubReporter) updateReview(ctx context.Context, review *github.PullRequestReview, summary Summary) error { + slog.Info("Updating pull request review", slog.String("repo", fmt.Sprintf("%s/%s", gr.owner, gr.repo))) - ctx, cancel := context.WithTimeout(context.Background(), gr.timeout) + reqCtx, cancel := context.WithTimeout(ctx, gr.timeout) defer cancel() - _, _, err := gr.client.PullRequests.CreateComment(ctx, gr.owner, gr.repo, gr.prNum, comment) + _, _, err := gr.client.PullRequests.UpdateReview( + reqCtx, + gr.owner, + gr.repo, + gr.prNum, + review.GetID(), + formatGHReviewBody(gr.version, summary), + ) return err } -func (gr GithubReporter) createReview(headCommit string, summary Summary) error { - slog.Info("Creating pull request review", slog.String("repo", fmt.Sprintf("%s/%s", gr.owner, gr.repo)), slog.String("commit", headCommit)) +func (gr GithubReporter) createReview(ctx context.Context, summary Summary) error { + slog.Info("Creating pull request review", slog.String("repo", fmt.Sprintf("%s/%s", gr.owner, gr.repo)), slog.String("commit", gr.headCommit)) - ctx, cancel := context.WithTimeout(context.Background(), gr.timeout) + reqCtx, cancel := context.WithTimeout(ctx, gr.timeout) defer cancel() _, resp, err := gr.client.PullRequests.CreateReview( - ctx, + reqCtx, gr.owner, gr.repo, gr.prNum, &github.PullRequestReviewRequest{ - CommitID: github.String(headCommit), + CommitID: github.String(gr.headCommit), Body: github.String(formatGHReviewBody(gr.version, summary)), Event: github.String("COMMENT"), }, @@ -300,53 +336,16 @@ func formatGHReviewBody(version string, summary Summary) string { return b.String() } -func reportToGitHubComment(headCommit string, rep Report) *github.PullRequestComment { - var msgPrefix, msgSuffix string - reportLine, srcLine := moveReportedLine(rep) - if reportLine != srcLine { - msgPrefix = fmt.Sprintf("Problem reported on unmodified line %d, comment moved here: ", srcLine) - } - if rep.Problem.Details != "" { - msgSuffix = "\n\n" + rep.Problem.Details - } - - var side string - if rep.Problem.Anchor == checks.AnchorBefore { - side = "LEFT" - } else { - side = "RIGHT" - } - - c := github.PullRequestComment{ - CommitID: github.String(headCommit), - Path: github.String(rep.Path.SymlinkTarget), - Body: github.String(fmt.Sprintf( - "%s [%s](https://cloudflare.github.io/pint/checks/%s.html): %s%s%s", - problemIcon(rep.Problem.Severity), - rep.Problem.Reporter, - rep.Problem.Reporter, - msgPrefix, - rep.Problem.Text, - msgSuffix, - )), - Line: github.Int(reportLine), - Side: github.String(side), - } - - return &c -} - -func (gr GithubReporter) tooManyComments(nrComments int) error { +func (gr GithubReporter) generalComment(ctx context.Context, body string) error { comment := github.IssueComment{ - Body: github.String(fmt.Sprintf(`This pint run would create %d comment(s), which is more than %d limit configured for pint. -%d comments were skipped and won't be visibile on this PR.`, nrComments, gr.maxComments, nrComments-gr.maxComments)), + Body: github.String(body), } slog.Debug("Creating PR comment", slog.String("body", comment.GetBody())) - ctx, cancel := context.WithTimeout(context.Background(), gr.timeout) + reqCtx, cancel := context.WithTimeout(ctx, gr.timeout) defer cancel() - _, _, err := gr.client.Issues.CreateComment(ctx, gr.owner, gr.repo, gr.prNum, &comment) + _, _, err := gr.client.Issues.CreateComment(reqCtx, gr.owner, gr.repo, gr.prNum, &comment) return err } diff --git a/internal/reporter/github_test.go b/internal/reporter/github_test.go index eb7ce0e8..0b63aaea 100644 --- a/internal/reporter/github_test.go +++ b/internal/reporter/github_test.go @@ -1,6 +1,7 @@ package reporter_test import ( + "context" "fmt" "io" "log/slog" @@ -15,7 +16,6 @@ import ( "github.com/cloudflare/pint/internal/checks" "github.com/cloudflare/pint/internal/discovery" - "github.com/cloudflare/pint/internal/git" "github.com/cloudflare/pint/internal/parser" "github.com/cloudflare/pint/internal/reporter" ) @@ -24,7 +24,6 @@ func TestGithubReporter(t *testing.T) { type testCaseT struct { httpHandler http.Handler error func(uri string) string - gitCmd git.CommandRunner description string @@ -45,16 +44,9 @@ func TestGithubReporter(t *testing.T) { expr: sum(errors) by (job) `)) - blameLine := func(sha string, line int, filename, content string) string { - return fmt.Sprintf(`%s %d %d 1 -filename %s - %s -`, sha, line, line, filename, content) - } - for _, tc := range []testCaseT{ { - description: "timeout errors out", + description: "list pull requests timeout", owner: "foo", repo: "bar", token: "something", @@ -65,16 +57,6 @@ filename %s _, _ = w.Write([]byte("OK")) }), timeout: 100 * time.Millisecond, - gitCmd: func(args ...string) ([]byte, error) { - if args[0] == "rev-parse" { - return []byte("fake-commit-id"), nil - } - if args[0] == "blame" { - content := blameLine("fake-commit-id", 2, "foo.txt", "up == 0") - return []byte(content), nil - } - return nil, nil - }, error: func(_ string) string { return "failed to list pull request reviews: context deadline exceeded" }, @@ -107,16 +89,6 @@ filename %s prNum: 123, maxComments: 50, timeout: time.Second, - gitCmd: func(args ...string) ([]byte, error) { - if args[0] == "rev-parse" { - return []byte("fake-commit-id"), nil - } - if args[0] == "blame" { - content := blameLine("fake-commit-id", 2, "foo.txt", "up == 0") - return []byte(content), nil - } - return nil, nil - }, httpHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/reviews" { w.WriteHeader(http.StatusBadRequest) @@ -150,6 +122,16 @@ filename %s }, }, }, + { + description: "no problems", + owner: "foo", + repo: "bar", + token: "something", + prNum: 123, + maxComments: 50, + timeout: time.Second, + reports: []reporter.Report{}, + }, { description: "happy path", owner: "foo", @@ -158,16 +140,6 @@ filename %s prNum: 123, maxComments: 50, timeout: time.Second, - gitCmd: func(args ...string) ([]byte, error) { - if args[0] == "rev-parse" { - return []byte("fake-commit-id"), nil - } - if args[0] == "blame" { - content := blameLine("fake-commit-id", 2, "foo.txt", "up == 0") - return []byte(content), nil - } - return nil, nil - }, reports: []reporter.Report{ { Path: discovery.Path{ @@ -198,16 +170,6 @@ filename %s prNum: 123, maxComments: 50, timeout: time.Second, - gitCmd: func(args ...string) ([]byte, error) { - if args[0] == "rev-parse" { - return []byte("fake-commit-id"), nil - } - if args[0] == "blame" { - content := blameLine("fake-commit-id", 2, "foo.txt", "up == 0") - return []byte(content), nil - } - return nil, nil - }, httpHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodPost && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/reviews" { w.WriteHeader(http.StatusBadGateway) @@ -249,16 +211,6 @@ filename %s prNum: 123, maxComments: 50, timeout: time.Second, - gitCmd: func(args ...string) ([]byte, error) { - if args[0] == "rev-parse" { - return []byte("fake-commit-id"), nil - } - if args[0] == "blame" { - content := blameLine("fake-commit-id", 2, "foo.txt", "up == 0") - return []byte(content), nil - } - return nil, nil - }, httpHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/reviews" { _, _ = w.Write([]byte(`[{"id":1,"body":"### This pull request was validated by [pint](https://github.com/cloudflare/pint).\nxxxx"}]`)) @@ -304,16 +256,6 @@ filename %s prNum: 123, maxComments: 50, timeout: time.Second, - gitCmd: func(args ...string) ([]byte, error) { - if args[0] == "rev-parse" { - return []byte("fake-commit-id"), nil - } - if args[0] == "blame" { - content := blameLine("fake-commit-id", 2, "foo.txt", "up == 0") - return []byte(content), nil - } - return nil, nil - }, httpHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/reviews" { _, _ = w.Write([]byte(`[{"id":1,"body":"### This pull request was validated by [pint](https://github.com/cloudflare/pint).\nxxxx"}]`)) @@ -355,16 +297,115 @@ filename %s prNum: 123, maxComments: 2, timeout: time.Second, - gitCmd: func(args ...string) ([]byte, error) { - if args[0] == "rev-parse" { - return []byte("fake-commit-id"), nil + httpHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/reviews" { + _, _ = w.Write([]byte(`[{"id":1,"body":"### This pull request was validated by [pint](https://github.com/cloudflare/pint).\nxxxx"}]`)) + return + } + if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/comments" { + _, _ = w.Write([]byte(`[{"id":1,"commit_id":"fake-commit-id","path":"foo.txt","line":2,"body":":stop_sign: [mock](https://cloudflare.github.io/pint/checks/mock.html): syntax error\n\nsyntax details"}]`)) + return } - if args[0] == "blame" { - content := blameLine("fake-commit-id", 2, "foo.txt", "up == 0") - return []byte(content), nil + if r.Method == http.MethodPost && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/comments" { + body, _ := io.ReadAll(r.Body) + b := strings.TrimSpace(strings.TrimRight(string(body), "\n\t\r")) + switch b { + case `{"body":":stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **mock1** check.\n\n------\n\nsyntax error1\n\nsyntax details1\n\n------\n\n:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/mock1.html).\n","path":"foo.txt","line":2,"side":"RIGHT","commit_id":"HEAD"}`: + case `{"body":":stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **mock2** check.\n\n------\n\nsyntax error2\n\nsyntax details2\n\n------\n\n:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/mock2.html).\n","path":"foo.txt","line":2,"side":"RIGHT","commit_id":"HEAD"}`: + case `{"body":"This pint run would create 4 comment(s), which is more than 2 limit configured for pint.\n2 comments were skipped and won't be visibile on this PR."}`: + default: + t.Errorf("Unexpected comment: %s", b) + } } - return nil, nil + _, _ = w.Write([]byte("")) + }), + reports: []reporter.Report{ + { + Path: discovery.Path{ + Name: "foo.txt", + SymlinkTarget: "foo.txt", + }, + + ModifiedLines: []int{2}, + Rule: mockRules[1], + Problem: checks.Problem{ + Lines: parser.LineRange{ + First: 2, + Last: 2, + }, + Reporter: "mock1", + Text: "syntax error1", + Details: "syntax details1", + Severity: checks.Bug, + }, + }, + { + Path: discovery.Path{ + Name: "foo.txt", + SymlinkTarget: "foo.txt", + }, + + ModifiedLines: []int{2}, + Rule: mockRules[1], + Problem: checks.Problem{ + Lines: parser.LineRange{ + First: 2, + Last: 2, + }, + Reporter: "mock2", + Text: "syntax error2", + Details: "syntax details2", + Severity: checks.Bug, + }, + }, + { + Path: discovery.Path{ + Name: "foo.txt", + SymlinkTarget: "foo.txt", + }, + + ModifiedLines: []int{2}, + Rule: mockRules[1], + Problem: checks.Problem{ + Lines: parser.LineRange{ + First: 2, + Last: 2, + }, + Reporter: "mock3", + Text: "syntax error3", + Details: "syntax details3", + Severity: checks.Fatal, + }, + }, + { + Path: discovery.Path{ + Name: "foo.txt", + SymlinkTarget: "foo.txt", + }, + + ModifiedLines: []int{2}, + Rule: mockRules[1], + Problem: checks.Problem{ + Lines: parser.LineRange{ + First: 2, + Last: 2, + }, + Reporter: "mock4", + Text: "syntax error4", + Details: "syntax details4", + Severity: checks.Fatal, + }, + }, }, + }, + { + description: "maxComments 2, too many comments comment error", + owner: "foo", + repo: "bar", + token: "something", + prNum: 123, + maxComments: 2, + timeout: time.Second, httpHandler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/reviews" { _, _ = w.Write([]byte(`[{"id":1,"body":"### This pull request was validated by [pint](https://github.com/cloudflare/pint).\nxxxx"}]`)) @@ -378,9 +419,12 @@ filename %s body, _ := io.ReadAll(r.Body) b := strings.TrimSpace(strings.TrimRight(string(body), "\n\t\r")) switch b { - case `{"body":":stop_sign: [mock1](https://cloudflare.github.io/pint/checks/mock1.html): syntax error1\n\nsyntax details1","path":"foo.txt","line":2,"side":"RIGHT","commit_id":"fake-commit-id"}`: - case `{"body":":stop_sign: [mock2](https://cloudflare.github.io/pint/checks/mock2.html): syntax error2\n\nsyntax details2","path":"foo.txt","line":2,"side":"RIGHT","commit_id":"fake-commit-id"}`: + case `{"body":":stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **mock1** check.\n\n------\n\nsyntax error1\n\nsyntax details1\n\n------\n\n:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/mock1.html).\n","path":"foo.txt","line":2,"side":"RIGHT","commit_id":"HEAD"}`: + case `{"body":":stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **mock2** check.\n\n------\n\nsyntax error2\n\nsyntax details2\n\n------\n\n:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/mock2.html).\n","path":"foo.txt","line":2,"side":"RIGHT","commit_id":"HEAD"}`: case `{"body":"This pint run would create 4 comment(s), which is more than 2 limit configured for pint.\n2 comments were skipped and won't be visibile on this PR."}`: + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte("Error")) + return default: t.Errorf("Unexpected comment: %s", b) } @@ -503,11 +547,11 @@ filename %s tc.repo, tc.prNum, tc.maxComments, - tc.gitCmd, + "HEAD", ) require.NoError(t, err) - err = r.Submit(reporter.NewSummary(tc.reports)) + err = reporter.Submit(context.Background(), reporter.NewSummary(tc.reports), r) if tc.error == nil { require.NoError(t, err) } else { diff --git a/internal/reporter/gitlab.go b/internal/reporter/gitlab.go index 6a06d16e..e6d1e427 100644 --- a/internal/reporter/gitlab.go +++ b/internal/reporter/gitlab.go @@ -138,24 +138,10 @@ func (gl GitLabReporter) Destinations(ctx context.Context) ([]any, error) { return dsts, nil } -func (gl GitLabReporter) Summary(ctx context.Context, dst any, s Summary, errs []error) error { +func (gl GitLabReporter) Summary(ctx context.Context, dst any, _ Summary, errs []error) error { mr := dst.(gitlabMR) - if gl.maxComments > 0 && len(s.reports) > gl.maxComments { - if err := gl.generalComment(ctx, mr.mrID, tooManyCommentsMsg(len(s.reports), gl.maxComments)); err != nil { - return err - } - } if len(errs) > 0 { - var buf strings.Builder - buf.WriteString("There were some errors when pint was reporting problems via GitLab APIs.\n") - buf.WriteString("Some review comments might be outdated or missing.\n") - buf.WriteString("List of all errors:\n\n") - for _, err := range errs { - buf.WriteString("- `") - buf.WriteString(err.Error()) - buf.WriteString("`\n") - } - if err := gl.generalComment(ctx, mr.mrID, buf.String()); err != nil { + if err := gl.generalComment(ctx, mr.mrID, errsToComment(errs)); err != nil { return err } } @@ -253,6 +239,10 @@ func (gl GitLabReporter) IsEqual(existing ExistingCommentV2, pending PendingComm return strings.Trim(existing.text, "\n") == strings.Trim(pending.text, "\n") } +func (gl GitLabReporter) CanDelete(ExistingCommentV2) bool { + return true +} + func (gl GitLabReporter) CanCreate(done int) (bool, error) { if done >= gl.maxComments { return false, errors.New(tooManyCommentsMsg(done, gl.maxComments)) @@ -474,10 +464,6 @@ func loggifyDiscussion(opt *gitlab.CreateMergeRequestDiscussionOptions) (attrs [ return attrs } -func (gl GitLabReporter) Submit(summary Summary) (err error) { - return Submit(context.Background(), summary, gl) -} - func tooManyCommentsMsg(nr, max int) string { return fmt.Sprintf(`This pint run would create %d comment(s), which is more than the limit configured for pint (%d). %d comment(s) were skipped and won't be visibile on this PR.`, nr, max, nr-max) diff --git a/internal/reporter/gitlab_test.go b/internal/reporter/gitlab_test.go index 55ebffb6..120bb6be 100644 --- a/internal/reporter/gitlab_test.go +++ b/internal/reporter/gitlab_test.go @@ -2,11 +2,9 @@ package reporter_test import ( "context" - "fmt" "log/slog" "net/http" "net/http/httptest" - "strings" "testing" "time" @@ -76,83 +74,6 @@ func TestGitLabReporter(t *testing.T) { fooDiff := `@@ -1,4 +1,6 @@\n- record: target is down\n- expr: up == 0\n+ expr: up == 1\n+ labels:\n+ foo: bar\n- record: sum errors\nexpr: sum(errors) by (job)\n` testCases := []testCaseT{ - { - description: "returns an error on non-200 HTTP response", - branch: "fakeBranch", - token: "fakeToken", - timeout: time.Second, - project: 123, - maxComments: 50, - reports: []reporter.Report{fooReport}, - httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(400) - _, _ = w.Write([]byte("Bad Request")) - }), - errorHandler: func(err error) error { - if err != nil && strings.HasPrefix(err.Error(), "failed to get GitLab user ID:") { - return nil - } - return fmt.Errorf("wrong error: %w", err) - }, - }, - { - description: "returns an error on HTTP response timeout", - branch: "fakeBranch", - token: "fakeToken", - timeout: time.Second, - project: 123, - maxComments: 50, - reports: []reporter.Report{fooReport}, - httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - time.Sleep(time.Second * 2) - w.WriteHeader(400) - _, _ = w.Write([]byte("Bad Request")) - }), - errorHandler: func(err error) error { - if err != nil && strings.HasSuffix(err.Error(), "context deadline exceeded") { - return nil - } - return fmt.Errorf("wrong error: %w", err) - }, - }, - { - description: "returns an error on non-json body", - branch: "fakeBranch", - token: "fakeToken", - timeout: time.Second, - project: 123, - maxComments: 50, - reports: []reporter.Report{fooReport}, - httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(200) - _, _ = w.Write([]byte("OK")) - }), - errorHandler: func(err error) error { - if err != nil && strings.HasPrefix(err.Error(), "failed to get GitLab user ID:") { - return nil - } - return fmt.Errorf("wrong error: %w", err) - }, - }, - { - description: "returns an error on empty JSON body", - branch: "fakeBranch", - token: "fakeToken", - timeout: time.Second, - project: 123, - maxComments: 50, - reports: []reporter.Report{fooReport}, - httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(200) - _, _ = w.Write([]byte("{}")) - }), - errorHandler: func(err error) error { - if err != nil && strings.HasPrefix(err.Error(), "failed to get GitLab merge request details: json: cannot unmarshal object into Go value of type") { - return nil - } - return fmt.Errorf("wrong error: %w", err) - }, - }, { description: "empty list of merge requests", branch: "fakeBranch", @@ -275,11 +196,9 @@ func TestGitLabReporter(t *testing.T) { if err == nil { summary := reporter.NewSummary(tc.reports) err = reporter.Submit(context.Background(), summary, r) + require.NoError(t, err) } - if e := tc.errorHandler(err); e != nil { - t.Errorf("error check failure: %s", e) - return - } + require.NoError(t, tc.errorHandler(err)) }) } } diff --git a/internal/reporter/reporter.go b/internal/reporter/reporter.go index 0ffcff12..8d0d44ef 100644 --- a/internal/reporter/reporter.go +++ b/internal/reporter/reporter.go @@ -1,14 +1,11 @@ package reporter import ( - "context" - "log/slog" "sort" "time" "github.com/cloudflare/pint/internal/checks" "github.com/cloudflare/pint/internal/discovery" - "github.com/cloudflare/pint/internal/output" "github.com/cloudflare/pint/internal/parser" ) @@ -126,106 +123,3 @@ func (s Summary) CountBySeverity() map[checks.Severity]int { type Reporter interface { Submit(Summary) error } - -func Submit(ctx context.Context, s Summary, c Commenter) error { - dsts, err := c.Destinations(ctx) - if err != nil { - return err - } - - for _, dst := range dsts { - if err = updateDestination(ctx, s, c, dst); err != nil { - return err - } - } - - return nil -} - -func updateDestination(ctx context.Context, s Summary, c Commenter, dst any) error { - slog.Info("Listing existing comments", slog.String("reporter", c.Describe())) - existingComments, err := c.List(ctx, dst) - if err != nil { - return err - } - - var created int - var ok bool - var errs []error - pendingComments := makeComments(s) - for _, pending := range pendingComments { - for _, existing := range existingComments { - if c.IsEqual(existing, pending) { - slog.Debug("Comment already exists", - slog.String("reporter", c.Describe()), - slog.String("path", pending.path), - slog.Int("line", pending.line), - ) - goto NEXTCreate - } - } - slog.Debug("Comment doesn't exist yet", - slog.String("reporter", c.Describe()), - slog.String("path", pending.path), - slog.Int("line", pending.line), - ) - - ok, err = c.CanCreate(created) - if err != nil { - errs = append(errs, err) - } - if !ok { - slog.Debug("Cannot create new comment", - slog.String("reporter", c.Describe()), - slog.String("path", pending.path), - slog.Int("line", pending.line), - ) - goto NEXTCreate - } - - if err := c.Create(ctx, dst, pending); err != nil { - slog.Error("Failed to create a new comment", - slog.String("reporter", c.Describe()), - slog.String("path", pending.path), - slog.Int("line", pending.line), - slog.Any("err", err), - ) - return err - } - created++ - NEXTCreate: - } - - for _, existing := range existingComments { - for _, pending := range pendingComments { - if c.IsEqual(existing, pending) { - goto NEXTDelete - } - } - if err := c.Delete(ctx, dst, existing); err != nil { - slog.Error("Failed to delete a stale comment", - slog.String("reporter", c.Describe()), - slog.String("path", existing.path), - slog.Int("line", existing.line), - slog.Any("err", err), - ) - errs = append(errs, err) - } - NEXTDelete: - } - - slog.Info("Creating report summary", - slog.String("reporter", c.Describe()), - slog.Int("reports", len(s.reports)), - slog.Int("online", int(s.OnlineChecks)), - slog.Int("offline", int(s.OnlineChecks)), - slog.String("duration", output.HumanizeDuration(s.Duration)), - slog.Int("entries", s.TotalEntries), - slog.Int("checked", int(s.CheckedEntries)), - ) - if err := c.Summary(ctx, dst, s, errs); err != nil { - return err - } - - return nil -}