Skip to content

Commit

Permalink
Merge pull request #1123 from cloudflare/gh
Browse files Browse the repository at this point in the history 
Don't create comments on unmodified lines
  • Loading branch information
@prymitive
prymitive authored Sep 30, 2024
2 parents e801589 + 34b4024 commit adb0966
Show file tree
Hide file tree
Showing 6 changed files with 240 additions and 26 deletions.
1 change: 1 addition & 0 deletions docs/changelog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Fixed

- Improved accuracy of the [rule/duplicate](checks/rule/duplicate.md) check.
- Fixed GitHub reporter trying to create pull request comments to unmodified lines - #1120.

## v0.65.1

Expand Down
6 changes: 3 additions & 3 deletions internal/reporter/comments.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ type Commenter interface {
Delete(context.Context, any, ExistingComment) error
CanCreate(int) bool
CanDelete(ExistingComment) bool
IsEqual(ExistingComment, PendingComment) bool
IsEqual(any, ExistingComment, PendingComment) bool
}

func NewCommentReporter(c Commenter) CommentReporter {
Expand Down Expand Up @@ -220,7 +220,7 @@ func updateDestination(ctx context.Context, s Summary, c Commenter, dst any) (er
slog.String("msg", pending.text),
)
for _, existing := range existingComments {
if c.IsEqual(existing, pending) {
if c.IsEqual(dst, existing, pending) {
slog.Debug("Comment already exists",
slog.String("reporter", c.Describe()),
slog.String("path", pending.path),
Expand Down Expand Up @@ -259,7 +259,7 @@ func updateDestination(ctx context.Context, s Summary, c Commenter, dst any) (er

for _, existing := range existingComments {
for _, pending := range pendingComments {
if c.IsEqual(existing, pending) {
if c.IsEqual(dst, existing, pending) {
goto NEXTDelete
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/reporter/comments_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (tc testCommenter) CanCreate(n int) bool {
return tc.canCreate(n)
}

func (tc testCommenter) IsEqual(e ExistingComment, p PendingComment) bool {
func (tc testCommenter) IsEqual(_ any, e ExistingComment, p PendingComment) bool {
return tc.isEqual(e, p)
}

Expand Down
86 changes: 68 additions & 18 deletions internal/reporter/github.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"context"
"fmt"
"log/slog"
"slices"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -39,7 +38,20 @@ type ghCommentMeta struct {
}

type ghPR struct {
paths []string
files []*github.CommitFile
}

func (pr ghPR) String() string {
return fmt.Sprintf("%d file(s)", len(pr.files))
}

func (pr ghPR) getFile(path string) *github.CommitFile {
for _, f := range pr.files {
if f.GetFilename() == path {
return f
}
}
return nil
}

// NewGithubReporter creates a new GitHub reporter that reports
Expand Down Expand Up @@ -91,7 +103,7 @@ func (gr GithubReporter) Describe() string {

func (gr GithubReporter) Destinations(ctx context.Context) (_ []any, err error) {
pr := ghPR{}
pr.paths, err = gr.listPRFiles(ctx)
pr.files, err = gr.listPRFiles(ctx)
return []any{pr}, err
}

Expand Down Expand Up @@ -154,25 +166,29 @@ func (gr GithubReporter) List(ctx context.Context, _ any) ([]ExistingComment, er
func (gr GithubReporter) Create(ctx context.Context, dst any, p PendingComment) error {
pr := dst.(ghPR)

if !slices.Contains(pr.paths, p.path) {
file := pr.getFile(p.path)
if file == nil {
slog.Debug("Skipping report for path with no changes",
slog.String("path", p.path),
)
return nil
}

var side string
if p.anchor == checks.AnchorBefore {
side = "LEFT"
} else {
side = "RIGHT"
diffs := parseDiffLines(file.GetPatch())
if len(diffs) == 0 {
slog.Debug("Skipping report for path with no diff",
slog.String("path", p.path),
)
return nil
}

side, line := gr.fixCommentLine(dst, p)

comment := &github.PullRequestComment{
CommitID: github.String(gr.headCommit),
Path: github.String(p.path),
Body: github.String(p.text),
Line: github.Int(p.line),
Line: github.Int(line),
Side: github.String(side),
}

Expand Down Expand Up @@ -203,11 +219,12 @@ func (gr GithubReporter) CanCreate(done int) bool {
return done < gr.maxComments
}

func (gr GithubReporter) IsEqual(existing ExistingComment, pending PendingComment) bool {
func (gr GithubReporter) IsEqual(dst any, existing ExistingComment, pending PendingComment) bool {
if existing.path != pending.path {
return false
}
if existing.line != pending.line {
_, line := gr.fixCommentLine(dst, pending)
if existing.line != line {
return false
}
return strings.Trim(existing.text, "\n") == strings.Trim(pending.text, "\n")
Expand Down Expand Up @@ -277,7 +294,7 @@ func (gr GithubReporter) createReview(ctx context.Context, summary Summary) erro
return nil
}

func (gr GithubReporter) listPRFiles(ctx context.Context) ([]string, error) {
func (gr GithubReporter) listPRFiles(ctx context.Context) ([]*github.CommitFile, error) {
reqCtx, cancel := gr.reqContext(ctx)
defer cancel()

Expand All @@ -286,11 +303,7 @@ func (gr GithubReporter) listPRFiles(ctx context.Context) ([]string, error) {
if err != nil {
return nil, fmt.Errorf("failed to list pull request files: %w", err)
}
paths := []string{}
for _, file := range files {
paths = append(paths, file.GetFilename())
}
return paths, nil
return files, nil
}

func formatGHReviewBody(version string, summary Summary) string {
Expand Down Expand Up @@ -388,3 +401,40 @@ func (gr GithubReporter) generalComment(ctx context.Context, body string) error
func (gr GithubReporter) reqContext(ctx context.Context) (context.Context, context.CancelFunc) {
return context.WithTimeout(context.WithValue(ctx, github.SleepUntilPrimaryRateLimitResetWhenRateLimited, true), gr.timeout)
}

func (gr GithubReporter) fixCommentLine(dst any, p PendingComment) (string, int) {
pr := dst.(ghPR)
file := pr.getFile(p.path)

var side string
if p.anchor == checks.AnchorBefore {
side = "LEFT"
} else {
side = "RIGHT"
}

line := p.line
diffs := parseDiffLines(file.GetPatch())
dl, ok := diffLineFor(diffs, p.line)
switch {
case ok && dl.wasModified && p.anchor == checks.AnchorAfter:
// Comment on new or modified line.
line = dl.new
case ok && dl.wasModified && p.anchor == checks.AnchorBefore:
// Comment on new or modified line.
line = dl.old
default:
// Comment on unmodified line.
// Find first modified line and put it there.
for _, d := range diffs {
if !d.wasModified {
continue
}
line = d.new
side = "RIGHT"
break
}
}

return side, line
}
163 changes: 163 additions & 0 deletions internal/reporter/github_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,169 @@ func TestGithubReporter(t *testing.T) {
},
},
},
{
description: "modified line",
owner: "foo",
repo: "bar",
token: "something",
prNum: 123,
maxComments: 50,
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(`[]`))
return
}
if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/comments" {
_, _ = w.Write([]byte(`[]`))
return
}
if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/files" {
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`[{"filename":"foo.txt", "patch": "@@ -1,4 +1,4 @@ - record: target is down\n- expr: up == 1\n+ expr: up == 0\n - record: sum errors\n expr: sum(errors) by (job)"}]`))
return
}
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"))
if b != `{"body":":stop_sign: **Fatal** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nsyntax error\n\nsyntax 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","path":"foo.txt","line":1,"side":"RIGHT","commit_id":"HEAD"}` {
t.Errorf("Unexpected comment: %s", b)
t.FailNow()
}
}
_, _ = w.Write([]byte(""))
}),
reports: []reporter.Report{
{
Path: discovery.Path{
Name: "foo.txt",
SymlinkTarget: "foo.txt",
},

ModifiedLines: []int{1},
Rule: mockRules[0],
Problem: checks.Problem{
Lines: parser.LineRange{
First: 1,
Last: 1,
},
Reporter: "mock",
Text: "syntax error",
Details: "syntax details",
Severity: checks.Fatal,
},
},
},
},
{
description: "unmodified line",
owner: "foo",
repo: "bar",
token: "something",
prNum: 123,
maxComments: 50,
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(`[]`))
return
}
if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/comments" {
_, _ = w.Write([]byte(`[]`))
return
}
if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/files" {
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`[{"filename":"foo.txt", "patch": "@@ -1,4 +1,4 @@ - record: target is down\n- expr: up == 1\n+ expr: up == 0\n - record: sum errors\n expr: sum(errors) by (job)"}]`))
return
}
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"))
if b != `{"body":":stop_sign: **Fatal** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nsyntax error\n\nsyntax 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","path":"foo.txt","line":1,"side":"RIGHT","commit_id":"HEAD"}` {
t.Errorf("Unexpected comment: %s", b)
t.FailNow()
}
}
_, _ = 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: "mock",
Text: "syntax error",
Details: "syntax details",
Severity: checks.Fatal,
},
},
},
},
{
description: "removed line",
owner: "foo",
repo: "bar",
token: "something",
prNum: 123,
maxComments: 50,
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(`[]`))
return
}
if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/comments" {
_, _ = w.Write([]byte(`[]`))
return
}
if r.Method == http.MethodGet && r.URL.Path == "/api/v3/repos/foo/bar/pulls/123/files" {
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`[{"filename":"foo.txt", "patch": "@@ -1,5 +1,4 @@\n - record: target is down\n expr: up == 0\n- labels: {}\n - record: sum errors\n expr: sum(errors) by (job)"}]`))
return
}
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"))
if b != `{"body":":stop_sign: **Fatal** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nsyntax error\n\nsyntax 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","path":"foo.txt","line":3,"side":"LEFT","commit_id":"HEAD"}` {
t.Errorf("Unexpected comment: %s", b)
t.FailNow()
}
}
_, _ = w.Write([]byte(""))
}),
reports: []reporter.Report{
{
Path: discovery.Path{
Name: "foo.txt",
SymlinkTarget: "foo.txt",
},

ModifiedLines: []int{3},
Rule: mockRules[0],
Problem: checks.Problem{
Lines: parser.LineRange{
First: 3,
Last: 3,
},
Reporter: "mock",
Text: "syntax error",
Details: "syntax details",
Severity: checks.Fatal,
Anchor: checks.AnchorBefore,
},
},
},
},
} {
t.Run(tc.description, func(t *testing.T) {
slog.SetDefault(slogt.New(t))
Expand Down
8 changes: 4 additions & 4 deletions internal/reporter/gitlab.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ func (gl GitLabReporter) Delete(ctx context.Context, dst any, comment ExistingCo
return err
}

func (gl GitLabReporter) IsEqual(existing ExistingComment, pending PendingComment) bool {
func (gl GitLabReporter) IsEqual(_ any, existing ExistingComment, pending PendingComment) bool {
if existing.path != pending.path {
return false
}
Expand Down Expand Up @@ -346,7 +346,7 @@ func reportToGitLabDiscussion(pending PendingComment, diffs []*gitlab.MergeReque
},
}

dl, ok := diffLineFor(parseDiffLines(diff), pending.line)
dl, ok := diffLineFor(parseDiffLines(diff.Diff), pending.line)
switch {
case !ok:
// No diffLine for this line, most likely unmodified ?.
Expand Down Expand Up @@ -393,10 +393,10 @@ func diffLineFor(lines []diffLine, line int) (diffLine, bool) {

var diffRe = regexp.MustCompile(`@@ \-(\d+),(\d+) \+(\d+),(\d+) @@`)

func parseDiffLines(diff *gitlab.MergeRequestDiff) (lines []diffLine) {
func parseDiffLines(diff string) (lines []diffLine) {
var oldLine, newLine int

sc := bufio.NewScanner(strings.NewReader(diff.Diff))
sc := bufio.NewScanner(strings.NewReader(diff))
for sc.Scan() {
line := sc.Text()
switch {
Expand Down

0 comments on commit adb0966

Please sign in to comment.