Skip to content

Commit

Permalink
Do not ignore paginated reponse from github PR Listfiles API
Browse files Browse the repository at this point in the history
Fix issue where only the first page (30 files max) was considered
  • Loading branch information
mach6 authored and foosinn committed Sep 4, 2020
1 parent ad6876a commit 55c8aef
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 11 deletions.
25 changes: 17 additions & 8 deletions plugin/scm_clients/github_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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)
}

Expand Down
45 changes: 42 additions & 3 deletions plugin/scm_clients/github_client_test.go
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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")
Expand Down

0 comments on commit 55c8aef

Please sign in to comment.