From 01d90ce2251ac37bbb889d556c9c3f3c5ccd0a4e Mon Sep 17 00:00:00 2001 From: Lukasz Mierzwa Date: Fri, 24 Nov 2023 15:06:51 +0000 Subject: [PATCH] Dedup bitbucket comments --- docs/changelog.md | 2 + internal/reporter/bitbucket_api.go | 74 +++++-- internal/reporter/bitbucket_test.go | 302 +++++++++++++++++++++++++++- 3 files changed, 357 insertions(+), 21 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 973eeb06..fa526acd 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -8,6 +8,8 @@ pull requests. Changes that would previously be invisible to pint, like modifying comments or moving the entire rules file, will now trigger checks for affected rules. +- pint will now try to create fewer BitBucket comments by merging multiple + problem reports into a single comment. ## v0.49.2 diff --git a/internal/reporter/bitbucket_api.go b/internal/reporter/bitbucket_api.go index 5a1c925f..ab6c50a6 100644 --- a/internal/reporter/bitbucket_api.go +++ b/internal/reporter/bitbucket_api.go @@ -540,14 +540,14 @@ func (bb bitBucketAPI) getPullRequestComments(pr *bitBucketPR) ([]bitBucketComme func (bb bitBucketAPI) makeComments(summary Summary, changes *bitBucketPRChanges) []BitBucketPendingComment { comments := []BitBucketPendingComment{} - for _, report := range summary.reports { - if _, ok := changes.pathModifiedLines[report.ReportedPath]; !ok { + for _, reports := range dedupReports(summary.reports) { + if _, ok := changes.pathModifiedLines[reports[0].ReportedPath]; !ok { continue } var buf strings.Builder var icon string - switch report.Problem.Severity { + switch reports[0].Problem.Severity { case checks.Fatal, checks.Bug: icon = ":stop_sign:" case checks.Warning: @@ -557,30 +557,33 @@ func (bb bitBucketAPI) makeComments(summary Summary, changes *bitBucketPRChanges } buf.WriteString(icon) buf.WriteString(" **") - buf.WriteString(report.Problem.Severity.String()) + buf.WriteString(reports[0].Problem.Severity.String()) buf.WriteString("** reported by [pint](https://cloudflare.github.io/pint/) **") - buf.WriteString(report.Problem.Reporter) - buf.WriteString("** check.") - buf.WriteString("\n\n------\n\n") - buf.WriteString(report.Problem.Text) - if report.Problem.Details != "" { + buf.WriteString(reports[0].Problem.Reporter) + buf.WriteString("** check.\n\n") + for _, report := range reports { + buf.WriteString("------\n\n") + buf.WriteString(report.Problem.Text) buf.WriteString("\n\n") - buf.WriteString(report.Problem.Details) - } - buf.WriteString("\n\n------\n\n") - if report.ReportedPath != report.SourcePath { - buf.WriteString(":leftwards_arrow_with_hook: This problem was detected on a symlinked file ") - buf.WriteRune('`') - buf.WriteString(report.SourcePath) - buf.WriteString("`.\n\n") + if report.Problem.Details != "" { + buf.WriteString(report.Problem.Details) + buf.WriteString("\n\n") + } + if report.ReportedPath != report.SourcePath { + buf.WriteString(":leftwards_arrow_with_hook: This problem was detected on a symlinked file ") + buf.WriteRune('`') + buf.WriteString(report.SourcePath) + buf.WriteString("`.\n\n") + } } + buf.WriteString("------\n\n") buf.WriteString(":information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/") - buf.WriteString(report.Problem.Reporter) + buf.WriteString(reports[0].Problem.Reporter) buf.WriteString(".html).\n") pending := pendingComment{ - path: report.ReportedPath, - line: report.Problem.Lines[0], + path: reports[0].ReportedPath, + line: reports[0].Problem.Lines[0], text: buf.String(), } comments = append(comments, pending.toBitBucketComment(changes)) @@ -702,3 +705,34 @@ func reportToAnnotation(report Report) BitBucketAnnotation { Link: fmt.Sprintf("https://cloudflare.github.io/pint/checks/%s.html", report.Problem.Reporter), } } + +func dedupReports(src []Report) (dst [][]Report) { + for _, report := range src { + index := -1 + for i, d := range dst { + if d[0].Problem.Severity != report.Problem.Severity { + continue + } + if d[0].Problem.Reporter != report.Problem.Reporter { + continue + } + if d[0].ReportedPath != report.ReportedPath { + continue + } + if d[0].SourcePath != report.SourcePath { + continue + } + if d[0].Problem.Lines[0] != report.Problem.Lines[0] { + continue + } + index = i + break + } + if index < 0 { + dst = append(dst, []Report{report}) + continue + } + dst[index] = append(dst[index], report) + } + return dst +} diff --git a/internal/reporter/bitbucket_test.go b/internal/reporter/bitbucket_test.go index a54c5d7b..4a28eb45 100644 --- a/internal/reporter/bitbucket_test.go +++ b/internal/reporter/bitbucket_test.go @@ -922,7 +922,7 @@ func TestBitBucketReporter(t *testing.T) { }, }, { - Text: ":warning: **Warning** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nmock text 2\n\n------\n\n:leftwards_arrow_with_hook: This problem was detected on a symlinked file `symlink.txt`.\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/mock.html).\n", + Text: ":warning: **Warning** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nmock text 2\n\n:leftwards_arrow_with_hook: This problem was detected on a symlinked file `symlink.txt`.\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/mock.html).\n", Severity: "NORMAL", Anchor: reporter.BitBucketPendingCommentAnchor{ Path: "foo.txt", @@ -1512,6 +1512,306 @@ func TestBitBucketReporter(t *testing.T) { return fmt.Errorf("Expected failed to create BitBucket pull request comments: POST request failed, got %w", err) }, }, + { + description: "sends a correct report with deduped comments", + gitCmd: fakeGit, + reports: []reporter.Report{ + { + ReportedPath: "foo.txt", + SourcePath: "foo.txt", + ModifiedLines: []int{2, 4}, + Rule: mockRules[1], + Problem: checks.Problem{ + Fragment: "up", + Lines: []int{1}, + Reporter: "mock", + Text: "this should be ignored, line is not part of the diff", + Severity: checks.Bug, + }, + }, + { + ReportedPath: "foo.txt", + SourcePath: "foo.txt", + ModifiedLines: []int{2, 4}, + Rule: mockRules[1], + Problem: checks.Problem{ + Fragment: "up", + Lines: []int{1}, + Reporter: "mock", + Text: "this should be ignored, line is not part of the diff", + Severity: checks.Bug, + }, + }, + { + ReportedPath: "foo.txt", + SourcePath: "foo.txt", + ModifiedLines: []int{2, 4}, + Rule: mockRules[1], + Problem: checks.Problem{ + Fragment: "up", + Lines: []int{2}, + Reporter: "mock", + Text: "bad name", + Details: "bad name details", + Severity: checks.Warning, + }, + }, + { + ReportedPath: "foo.txt", + SourcePath: "foo.txt", + ModifiedLines: []int{2, 4}, + Rule: mockRules[0], + Problem: checks.Problem{ + Fragment: "up == 0", + Lines: []int{2}, + Reporter: "mock", + Text: "mock text 1", + Details: "mock details", + Severity: checks.Warning, + }, + }, + { + ReportedPath: "foo.txt", + SourcePath: "symlink.txt", + ModifiedLines: []int{2, 4}, + Rule: mockRules[1], + Problem: checks.Problem{ + Fragment: "errors", + Lines: []int{2}, + Reporter: "mock", + Text: "mock text 2", + Details: "mock details", + Severity: checks.Warning, + }, + }, + }, + report: reporter.BitBucketReport{ + Reporter: "Prometheus rule linter", + Title: "pint v0.0.0", + Details: reporter.BitBucketDescription, + Link: "https://cloudflare.github.io/pint/", + Result: "FAIL", + Data: []reporter.BitBucketReportData{ + {Title: "Number of rules checked", Type: reporter.NumberType, Value: float64(0)}, + {Title: "Number of problems found", Type: reporter.NumberType, Value: float64(5)}, + {Title: "Number of offline checks", Type: reporter.NumberType, Value: float64(0)}, + {Title: "Number of online checks", Type: reporter.NumberType, Value: float64(0)}, + {Title: "Checks duration", Type: reporter.DurationType, Value: float64(0)}, + }, + }, + pullRequestComments: []reporter.BitBucketPendingComment{ + { + Text: ":stop_sign: **Bug** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nthis should be ignored, line is not part of the diff\n\n------\n\nthis should be ignored, line is not part of the diff\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/mock.html).\n", + Severity: "NORMAL", + Anchor: reporter.BitBucketPendingCommentAnchor{ + Path: "foo.txt", + Line: 1, + LineType: "CONTEXT", + FileType: "FROM", + DiffType: "EFFECTIVE", + }, + }, + { + Text: ":warning: **Warning** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nbad name\n\nbad name details\n\n------\n\nmock text 1\n\nmock details\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/mock.html).\n", + Severity: "NORMAL", + Anchor: reporter.BitBucketPendingCommentAnchor{ + Path: "foo.txt", + Line: 2, + LineType: "ADDED", + FileType: "TO", + DiffType: "EFFECTIVE", + }, + }, + { + Text: ":warning: **Warning** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nmock text 2\n\nmock details\n\n:leftwards_arrow_with_hook: This problem was detected on a symlinked file `symlink.txt`.\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/mock.html).\n", + Severity: "NORMAL", + Anchor: reporter.BitBucketPendingCommentAnchor{ + Path: "foo.txt", + Line: 2, + LineType: "ADDED", + FileType: "TO", + DiffType: "EFFECTIVE", + }, + }, + }, + pullRequests: reporter.BitBucketPullRequests{ + IsLastPage: true, + Values: []reporter.BitBucketPullRequest{ + { + ID: 102, + Open: true, + FromRef: reporter.BitBucketRef{ + ID: "refs/heads/fake-branch", + Commit: "fake-commit-id", + }, + ToRef: reporter.BitBucketRef{ + ID: "refs/heads/main", + Commit: "main-commit-id", + }, + }, + }, + }, + pullRequestChanges: reporter.BitBucketPullRequestChanges{ + IsLastPage: true, + Values: []reporter.BitBucketPullRequestChange{ + { + Path: reporter.BitBucketPath{ + ToString: "index.txt", + }, + }, + { + Path: reporter.BitBucketPath{ + ToString: "foo.txt", + }, + }, + }, + }, + pullRequestFileDiffs: map[string]reporter.BitBucketFileDiffs{ + "index.txt": { + Diffs: []reporter.BitBucketFileDiff{ + { + Hunks: []reporter.BitBucketDiffHunk{ + { + Segments: []reporter.BitBucketDiffSegment{ + { + Type: "ADDED", + Lines: []reporter.BitBucketDiffLine{ + {Source: 1, Destination: 1}, + {Source: 5, Destination: 5}, + }, + }, + { + Type: "CONTEXT", + Lines: []reporter.BitBucketDiffLine{ + {Source: 10, Destination: 6}, + }, + }, + }, + }, + }, + }, + }, + }, + "foo.txt": { + Diffs: []reporter.BitBucketFileDiff{ + { + Hunks: []reporter.BitBucketDiffHunk{ + { + Segments: []reporter.BitBucketDiffSegment{ + { + Type: "ADDED", + Lines: []reporter.BitBucketDiffLine{ + {Source: 2, Destination: 2}, + }, + }, + }, + }, + }, + }, + { + Hunks: []reporter.BitBucketDiffHunk{ + { + Segments: []reporter.BitBucketDiffSegment{ + { + Type: "MODIFIED", + Lines: []reporter.BitBucketDiffLine{ + {Source: 3, Destination: 4}, + }, + }, + }, + }, + }, + }, + }, + }, + }, + pullRequestActivities: reporter.BitBucketPullRequestActivities{ + IsLastPage: true, + Values: []reporter.BitBucketPullRequestActivity{ + { + Action: "APPROVED", + }, + { + Action: "COMMENTED", + CommentAction: "ADDED", + CommentAnchor: reporter.BitBucketCommentAnchor{ + Orphaned: true, + DiffType: "EFFECTIVE", + Path: "foo.txt", + Line: 3, + }, + Comment: reporter.BitBucketPullRequestComment{ + ID: 1001, + Version: 0, + State: "OPEN", + Author: reporter.BitBucketCommentAuthor{ + Name: "pint_user", + }, + }, + }, + { + Action: "COMMENTED", + CommentAction: "ADDED", + CommentAnchor: reporter.BitBucketCommentAnchor{ + Orphaned: true, + DiffType: "COMMIT", + Path: "foo.txt", + Line: 10, + }, + Comment: reporter.BitBucketPullRequestComment{ + ID: 1002, + Version: 1, + State: "OPEN", + Author: reporter.BitBucketCommentAuthor{ + Name: "pint_user", + }, + }, + }, + { + Action: "COMMENTED", + CommentAction: "ADDED", + CommentAnchor: reporter.BitBucketCommentAnchor{ + Orphaned: false, + DiffType: "EFFECTIVE", + Path: "foo.txt", + Line: 3, + }, + Comment: reporter.BitBucketPullRequestComment{ + ID: 2001, + Version: 0, + State: "OPEN", + Author: reporter.BitBucketCommentAuthor{ + Name: "pint_user", + }, + }, + }, + { + Action: "COMMENTED", + CommentAction: "ADDED", + CommentAnchor: reporter.BitBucketCommentAnchor{ + Orphaned: false, + DiffType: "COMMIT", + Path: "foo.txt", + Line: 4, + }, + Comment: reporter.BitBucketPullRequestComment{ + ID: 2002, + Version: 1, + State: "OPEN", + Author: reporter.BitBucketCommentAuthor{ + Name: "pint_user", + }, + }, + }, + }, + }, + errorHandler: func(err error) error { + if err != nil { + return fmt.Errorf("Unpexpected error: %w", err) + } + return nil + }, + }, } for _, tc := range testCases {