From 55c8aefd55a5c385ded8f271c6e1a8b86566d68b Mon Sep 17 00:00:00 2001 From: Doug Simmons Date: Mon, 17 Aug 2020 16:46:22 -0700 Subject: [PATCH] Do not ignore paginated reponse from github PR Listfiles API Fix issue where only the first page (30 files max) was considered --- plugin/scm_clients/github_client.go | 25 ++++++++----- plugin/scm_clients/github_client_test.go | 45 ++++++++++++++++++++++-- 2 files changed, 59 insertions(+), 11 deletions(-) diff --git a/plugin/scm_clients/github_client.go b/plugin/scm_clients/github_client.go index 948eadd..bcad2f4 100644 --- a/plugin/scm_clients/github_client.go +++ b/plugin/scm_clients/github_client.go @@ -3,6 +3,7 @@ package scm_clients import ( "context" "fmt" + "github.com/drone/drone-go/drone" "github.com/google/go-github/github" "github.com/google/uuid" @@ -38,13 +39,22 @@ func NewGitHubClient(ctx context.Context, uuid uuid.UUID, server string, token s func (s GithubClient) ChangedFilesInPullRequest(ctx context.Context, pullRequestID int) ([]string, error) { var changedFiles []string - files, _, err := s.listFiles(ctx, pullRequestID) - if err != nil { - return nil, err - } - for _, file := range files { - changedFiles = append(changedFiles, *file.Filename) + opts := &github.ListOptions{} + + for { + files, resp, err := s.listFiles(ctx, pullRequestID, opts) + if err != nil { + return nil, err + } + for _, file := range files { + changedFiles = append(changedFiles, *file.Filename) + } + if resp.NextPage == 0 { + break + } + opts.Page = resp.NextPage } + return changedFiles, nil } @@ -91,9 +101,8 @@ func (s GithubClient) GetFileListing(ctx context.Context, path string, commitRef return result, err } -func (s GithubClient) listFiles(ctx context.Context, number int) ( +func (s GithubClient) listFiles(ctx context.Context, number int, opts *github.ListOptions) ( []*github.CommitFile, *github.Response, error) { - opts := &github.ListOptions{} return s.delegate.PullRequests.ListFiles(ctx, s.repo.Namespace, s.repo.Name, number, opts) } diff --git a/plugin/scm_clients/github_client_test.go b/plugin/scm_clients/github_client_test.go index 97d3b37..65987ac 100644 --- a/plugin/scm_clients/github_client_test.go +++ b/plugin/scm_clients/github_client_test.go @@ -1,14 +1,17 @@ package scm_clients import ( - "github.com/drone/drone-go/drone" - "github.com/google/uuid" - "github.com/sirupsen/logrus" + "fmt" "io" "net/http" "net/http/httptest" "os" + "reflect" "testing" + + "github.com/drone/drone-go/drone" + "github.com/google/uuid" + "github.com/sirupsen/logrus" ) const mockGithubToken = "7535706b694c63526c6e4f5230374243" @@ -47,6 +50,31 @@ func TestGithubClient_ChangedFilesInPullRequest(t *testing.T) { BaseTest_ChangedFilesInPullRequest(t, client) } +func TestGithubClient_ChangedFilesInPullRequest_Paginated(t *testing.T) { + ts := httptest.NewServer(testMuxGithub()) + defer ts.Close() + client, err := createGithubClient(ts.URL) + if err != nil { + t.Error(err) + return + } + + actualFiles, err := client.ChangedFilesInPullRequest(noContext, 4) + if err != nil { + t.Error(err) + return + } + + expectedFiles := []string{ + "e/f/g/h/.drone.yml", + "e/f/g/h/.drone.yml", + } + + if want, got := expectedFiles, actualFiles; !reflect.DeepEqual(want, got) { + t.Errorf("Test failed:\n want %q\n got %q", want, got) + } +} + func TestGithubClient_GetFileListing(t *testing.T) { ts := httptest.NewServer(testMuxGithub()) defer ts.Close() @@ -96,6 +124,17 @@ func testMuxGithub() *http.ServeMux { f, _ := os.Open("../testdata/github/pull_3_files.json") _, _ = io.Copy(w, f) }) + mux.HandleFunc("/repos/foosinn/dronetest/pulls/4/files", + func(w http.ResponseWriter, r *http.Request) { + // simulate a paginated response + if r.FormValue("page") == "" { + next := fmt.Sprintf("<%s?page=2>; rel=\"next\"", r.URL.String()) + last := fmt.Sprintf("<%s?page=2>; rel=\"last\"", r.URL.String()) + w.Header().Add("Link", fmt.Sprintf("%s, %s", next, last)) + } + f, _ := os.Open("../testdata/github/pull_3_files.json") + _, _ = io.Copy(w, f) + }) mux.HandleFunc("/repos/foosinn/dronetest/contents/afolder/.drone.yml", func(w http.ResponseWriter, r *http.Request) { f, _ := os.Open("../testdata/github/afolder_.drone.yml.json")