Merge pull request #1080 from cloudflare/gh

Don't try to report problems on unmodified files

cmd/pint/tests/0032_ci_github.txt

@@ -1,3 +1,4 @@
+http method github GET /api/v3/repos/cloudflare/pint/pulls/1/files 200 [{"filename":"rules.yml"}]
 http method github GET /api/v3/repos/cloudflare/pint/pulls/1/reviews 200 []
 http method github POST /api/v3/repos/cloudflare/pint/pulls/1/reviews 200 {}
 http method github GET /api/v3/repos/cloudflare/pint/pulls/1/comments 200 []

cmd/pint/tests/0033_ci_github_multi.txt

@@ -1,3 +1,4 @@
+http method github GET /api/v3/repos/cloudflare/pint/pulls/1/files 200 [{"filename":"rules.yml"}]
 http method github GET /api/v3/repos/cloudflare/pint/pulls/1/reviews 200 []
 http method github POST /api/v3/repos/cloudflare/pint/pulls/1/reviews 200 {}
 http method github GET /api/v3/repos/cloudflare/pint/pulls/1/comments 200 []

cmd/pint/tests/0083_github_action.txt

@@ -1,3 +1,4 @@
+http method github GET /api/v3/repos/foo/bar/pulls/123/files 200 [{"filename":"rules.yml"}]
 http method github GET /api/v3/repos/foo/bar/pulls/123/reviews 200 []
 http method github POST /api/v3/repos/foo/bar/pulls/123/reviews 200 {}
 http method github GET /api/v3/repos/foo/bar/pulls/123/comments 200 []

cmd/pint/tests/0084_github_action_override.txt

@@ -1,3 +1,4 @@
+http method github GET /api/v3/repos/cloudflare/pint/pulls/123/files 200 [{"filename":"rules.yml"}]
 http method github GET /api/v3/repos/cloudflare/pint/pulls/123/reviews 200 []
 http method github POST /api/v3/repos/cloudflare/pint/pulls/123/reviews 200 {}
 http method github GET /api/v3/repos/cloudflare/pint/pulls/123/comments 200 []

cmd/pint/tests/0085_github_no_envs.txt

@@ -1,3 +1,4 @@
+http method github GET /api/v3/repos/cloudflare/pint/pulls/123/files 200 [{"filename":"rules.yml"}]
 http method github GET /api/v3/repos/cloudflare/pint/pulls/123/reviews 200 []
 http method github POST /api/v3/repos/cloudflare/pint/pulls/123/reviews 200 {}
 http method github GET /api/v3/repos/cloudflare/pint/pulls/123/comments 200 []

cmd/pint/tests/0098_rule_file_symlink_gh.txt

@@ -1,3 +1,4 @@
+http method github GET /api/v3/repos/cloudflare/pint/pulls/1/files 200 [{"filename":"rules.yml"}]
 http method github GET /api/v3/repos/cloudflare/pint/pulls/1/reviews 200 []
 http method github POST /api/v3/repos/cloudflare/pint/pulls/1/reviews 200 {}
 http method github GET /api/v3/repos/cloudflare/pint/pulls/1/comments 200 []

cmd/pint/tests/0118_ci_dir_move.txt

@@ -1,3 +1,4 @@
+http method github GET /api/v3/repos/cloudflare/pint/pulls/1/files 200 [{"filename":"rules.yml"}]
 http method github GET /api/v3/repos/cloudflare/pint/pulls/1/reviews 200 []
 http method github POST /api/v3/repos/cloudflare/pint/pulls/1/reviews 200 {}
 http method github GET /api/v3/repos/cloudflare/pint/pulls/1/comments 200 []

cmd/pint/tests/0119_ci_fail_on_warning.txt

@@ -1,3 +1,4 @@
+http method github GET /api/v3/repos/cloudflare/pint/pulls/1/files 200 [{"filename":"rules.yml"}]
 http method github GET /api/v3/repos/cloudflare/pint/pulls/1/reviews 200 []
 http method github POST /api/v3/repos/cloudflare/pint/pulls/1/reviews 200 {}
 http method github GET /api/v3/repos/cloudflare/pint/pulls/1/comments 200 []

docs/changelog.md

@@ -9,6 +9,10 @@
 - [promql/range_query](checks/promql/range_query.md) now allows to configure a custom maximum
   duration for range queries - #1064.
 
+### Fixed
+
+- Don't try to report problem on unmodified files when using GitHub reporter.
+
 ## v0.64.1
 
 ### Fixed

internal/reporter/github.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"fmt"
 	"log/slog"
+	"slices"
 	"strconv"
 	"strings"
 	"time"
@@ -37,6 +38,10 @@ type ghCommentMeta struct {
 	id int64
 }
 
+type ghPR struct {
+	paths []string
+}
+
 // 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, headCommit string) (_ GithubReporter, err error) {
@@ -84,8 +89,10 @@ func (gr GithubReporter) Describe() string {
 	return "GitHub"
 }
 
-func (gr GithubReporter) Destinations(context.Context) ([]any, error) {
-	return []any{gr.prNum}, nil
+func (gr GithubReporter) Destinations(ctx context.Context) (_ []any, err error) {
+	pr := ghPR{}
+	pr.paths, err = gr.listPRFiles(ctx)
+	return []any{pr}, err
 }
 
 func (gr GithubReporter) Summary(ctx context.Context, _ any, s Summary, errs []error) error {
@@ -144,7 +151,16 @@ func (gr GithubReporter) List(ctx context.Context, _ any) ([]ExistingComment, er
 	return comments, nil
 }
 
-func (gr GithubReporter) Create(ctx context.Context, _ any, p PendingComment) error {
+func (gr GithubReporter) Create(ctx context.Context, dst any, p PendingComment) error {
+	pr := dst.(ghPR)
+
+	if !slices.Contains(pr.paths, p.path) {
+		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"
@@ -250,6 +266,22 @@ func (gr GithubReporter) createReview(ctx context.Context, summary Summary) erro
 	return nil
 }
 
+func (gr GithubReporter) listPRFiles(ctx context.Context) ([]string, error) {
+	reqCtx, cancel := gr.reqContext(ctx)
+	defer cancel()
+
+	slog.Debug("Getting the list of modified files", slog.Int("pr", gr.prNum))
+	files, _, err := gr.client.PullRequests.ListFiles(reqCtx, gr.owner, gr.repo, gr.prNum, nil)
+	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
+}
+
 func formatGHReviewBody(version string, summary Summary) string {
 	var b strings.Builder
 

internal/reporter/github_test.go

@@ -46,13 +46,59 @@ func TestGithubReporter(t *testing.T) {
 
 	for _, tc := range []testCaseT{
 		{
-			description: "list pull requests timeout",
+			description: "list files error",
 			owner:       "foo",
 			repo:        "bar",
 			token:       "something",
 			prNum:       123,
 			maxComments: 50,
-			httpHandler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			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/files" {
+					w.WriteHeader(http.StatusBadRequest)
+					_, _ = w.Write([]byte("Error"))
+					return
+				}
+				_, _ = w.Write([]byte(""))
+			}),
+			error: func(uri string) string {
+				return fmt.Sprintf("failed to list pull request files: GET %s/api/v3/repos/foo/bar/pulls/123/files: 400  []", uri)
+			},
+			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: "list pull reviews timeout",
+			owner:       "foo",
+			repo:        "bar",
+			token:       "something",
+			prNum:       123,
+			maxComments: 50,
+			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/files" {
+					w.WriteHeader(http.StatusOK)
+					_, _ = w.Write([]byte("[]"))
+					return
+				}
 				time.Sleep(1 * time.Second)
 				_, _ = w.Write([]byte("OK"))
 			}),
@@ -514,6 +560,11 @@ func TestGithubReporter(t *testing.T) {
 			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/files" {
+					w.WriteHeader(http.StatusOK)
+					_, _ = w.Write([]byte(`[{"filename":"foo.txt"}]`))
+					return
+				}
 				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