diff --git a/check.go b/check.go index 64df9e24..66c79de1 100644 --- a/check.go +++ b/check.go @@ -5,6 +5,7 @@ import ( "path/filepath" "regexp" "sort" + "strconv" "strings" "github.com/shurcooL/githubv4" @@ -83,44 +84,27 @@ Loop: continue } - // Fetch files once if paths/ignore_paths are specified. - var files []string - + // Filter pull request if paths or ignorePaths is specified and no wanted paths were found if len(request.Source.Paths) > 0 || len(request.Source.IgnorePaths) > 0 { - files, err = manager.ListModifiedFiles(p.Number) + found, err := HasWantedFiles( + strconv.Itoa(p.Number), + request.Source.Paths, + request.Source.IgnorePaths, + p.Files, + p.FilesPageInfo.HasNextPage, + string(p.FilesPageInfo.EndCursor), + manager, + ) + if err != nil { - return nil, fmt.Errorf("failed to list modified files: %s", err) + return nil, err } - } - // Skip version if no files match the specified paths. - if len(request.Source.Paths) > 0 { - var wanted []string - for _, pattern := range request.Source.Paths { - w, err := FilterPath(files, pattern) - if err != nil { - return nil, fmt.Errorf("path match failed: %s", err) - } - wanted = append(wanted, w...) - } - if len(wanted) == 0 { + if !found { continue Loop } } - // Skip version if all files are ignored. - if len(request.Source.IgnorePaths) > 0 { - wanted := files - for _, pattern := range request.Source.IgnorePaths { - wanted, err = FilterIgnorePath(wanted, pattern) - if err != nil { - return nil, fmt.Errorf("ignore path match failed: %s", err) - } - } - if len(wanted) == 0 { - continue Loop - } - } response = append(response, NewVersion(p)) } @@ -138,6 +122,55 @@ Loop: return response, nil } +func HasWantedFiles(prNumber string, paths []string, ignorePaths []string, files []ChangedFileObject, hasMoreFiles bool, nextFileCursor string, manager Github) (bool, error) { + // construct a slice that contains 'wanted' files and use this to determine if we should continue + // files are wanted either when they appear in the paths list or don't appear in the ignore paths list + var wanted []ChangedFileObject + var err error + + if len(paths) > 0 { + for _, pattern := range paths { + w, err := FilterPath(files, pattern) + if err != nil { + return false, fmt.Errorf("path match failed: %s", err) + } + wanted = append(wanted, w...) + } + } else { + wanted = files + } + + for _, pattern := range ignorePaths { + wanted, err = FilterIgnorePath(wanted, pattern) + if err != nil { + return false, fmt.Errorf("ignore path match failed: %s", err) + } + } + + if len(wanted) > 0 { + // wanted files were found + return true, nil + } + + if !hasMoreFiles { + // no wanted files were found and there are no more files to examine + return false, nil + } + + // no wanted files were found, but there are more files to check + // fetch them now and then check them in another iteration of this function + files, hasMoreFiles, nextFileCursor, err = manager.GetChangedFiles( + prNumber, + 100, + nextFileCursor, + ) + if err != nil { + return false, fmt.Errorf("get more files failed: %s", err) + } + + return HasWantedFiles(prNumber, paths, ignorePaths, files, hasMoreFiles, nextFileCursor, manager) +} + // ContainsSkipCI returns true if a string contains [ci skip] or [skip ci]. func ContainsSkipCI(s string) bool { re := regexp.MustCompile("(?i)\\[(ci skip|skip ci)\\]") @@ -145,30 +178,32 @@ func ContainsSkipCI(s string) bool { } // FilterIgnorePath ... -func FilterIgnorePath(files []string, pattern string) ([]string, error) { - var out []string - for _, file := range files { +func FilterIgnorePath(files []ChangedFileObject, pattern string) ([]ChangedFileObject, error) { + var out []ChangedFileObject + for _, cfo := range files { + file := cfo.Path match, err := filepath.Match(pattern, file) if err != nil { return nil, err } if !match && !IsInsidePath(pattern, file) { - out = append(out, file) + out = append(out, cfo) } } return out, nil } // FilterPath ... -func FilterPath(files []string, pattern string) ([]string, error) { - var out []string - for _, file := range files { +func FilterPath(files []ChangedFileObject, pattern string) ([]ChangedFileObject, error) { + var out []ChangedFileObject + for _, cfo := range files { + file := cfo.Path match, err := filepath.Match(pattern, file) if err != nil { return nil, err } if match || IsInsidePath(pattern, file) { - out = append(out, file) + out = append(out, cfo) } } return out, nil diff --git a/check_test.go b/check_test.go index 8c422914..be893a37 100644 --- a/check_test.go +++ b/check_test.go @@ -10,19 +10,25 @@ import ( ) var ( - testPullRequests = []*resource.PullRequest{ - createTestPR(1, "master", true, false, 0, nil, false, githubv4.PullRequestStateOpen), - createTestPR(2, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen), - createTestPR(3, "master", false, false, 0, nil, true, githubv4.PullRequestStateOpen), - createTestPR(4, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen), - createTestPR(5, "master", false, true, 0, nil, false, githubv4.PullRequestStateOpen), - createTestPR(6, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen), - createTestPR(7, "develop", false, false, 0, []string{"enhancement"}, false, githubv4.PullRequestStateOpen), - createTestPR(8, "master", false, false, 1, []string{"wontfix"}, false, githubv4.PullRequestStateOpen), - createTestPR(9, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen), - createTestPR(10, "master", false, false, 0, nil, false, githubv4.PullRequestStateClosed), - createTestPR(11, "master", false, false, 0, nil, false, githubv4.PullRequestStateMerged), - createTestPR(12, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen), + testPullRequests = []*CheckTestPR{ + createCheckTestPR(1, "master", true, false, 0, nil, false, githubv4.PullRequestStateOpen, [][]resource.ChangedFileObject{}), + createCheckTestPR(2, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen, [][]resource.ChangedFileObject{ + {{Path: "README.md"}, {Path: "travis.yml"}}, + }), + createCheckTestPR(3, "master", false, false, 0, nil, true, githubv4.PullRequestStateOpen, [][]resource.ChangedFileObject{ + {{Path: "terraform/modules/ecs/main.tf"}, {Path: "README.md"}}, + }), + createCheckTestPR(4, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen, [][]resource.ChangedFileObject{ + {{Path: "terraform/modules/variables.tf"}, {Path: "travis.yml"}}, + }), + createCheckTestPR(5, "master", false, true, 0, nil, false, githubv4.PullRequestStateOpen, [][]resource.ChangedFileObject{}), + createCheckTestPR(6, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen, [][]resource.ChangedFileObject{}), + createCheckTestPR(7, "develop", false, false, 0, []string{"enhancement"}, false, githubv4.PullRequestStateOpen, [][]resource.ChangedFileObject{}), + createCheckTestPR(8, "master", false, false, 1, []string{"wontfix"}, false, githubv4.PullRequestStateOpen, [][]resource.ChangedFileObject{}), + createCheckTestPR(9, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen, [][]resource.ChangedFileObject{}), + createCheckTestPR(10, "master", false, false, 0, nil, false, githubv4.PullRequestStateClosed, [][]resource.ChangedFileObject{}), + createCheckTestPR(11, "master", false, false, 0, nil, false, githubv4.PullRequestStateMerged, [][]resource.ChangedFileObject{}), + createCheckTestPR(12, "master", false, false, 0, nil, false, githubv4.PullRequestStateOpen, [][]resource.ChangedFileObject{}), } ) @@ -31,8 +37,7 @@ func TestCheck(t *testing.T) { description string source resource.Source version resource.Version - files [][]string - pullRequests []*resource.PullRequest + pullRequests []*CheckTestPR expected resource.CheckResponse }{ { @@ -43,9 +48,8 @@ func TestCheck(t *testing.T) { }, version: resource.Version{}, pullRequests: testPullRequests, - files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[1]), + resource.NewVersion(testPullRequests[1].PR), }, }, @@ -55,11 +59,10 @@ func TestCheck(t *testing.T) { Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken", }, - version: resource.NewVersion(testPullRequests[1]), + version: resource.NewVersion(testPullRequests[1].PR), pullRequests: testPullRequests, - files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[1]), + resource.NewVersion(testPullRequests[1].PR), }, }, @@ -69,12 +72,11 @@ func TestCheck(t *testing.T) { Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken", }, - version: resource.NewVersion(testPullRequests[3]), + version: resource.NewVersion(testPullRequests[3].PR), pullRequests: testPullRequests, - files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[2]), - resource.NewVersion(testPullRequests[1]), + resource.NewVersion(testPullRequests[2].PR), + resource.NewVersion(testPullRequests[1].PR), }, }, @@ -85,15 +87,10 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", Paths: []string{"terraform/*/*.tf", "terraform/*/*/*.tf"}, }, - version: resource.NewVersion(testPullRequests[3]), + version: resource.NewVersion(testPullRequests[3].PR), pullRequests: testPullRequests, - files: [][]string{ - {"README.md", "travis.yml"}, - {"terraform/modules/ecs/main.tf", "README.md"}, - {"terraform/modules/variables.tf", "travis.yml"}, - }, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[2]), + resource.NewVersion(testPullRequests[2].PR), }, }, @@ -104,15 +101,10 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", IgnorePaths: []string{"*.md", "*.yml"}, }, - version: resource.NewVersion(testPullRequests[3]), + version: resource.NewVersion(testPullRequests[3].PR), pullRequests: testPullRequests, - files: [][]string{ - {"README.md", "travis.yml"}, - {"terraform/modules/ecs/main.tf", "README.md"}, - {"terraform/modules/variables.tf", "travis.yml"}, - }, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[2]), + resource.NewVersion(testPullRequests[2].PR), }, }, @@ -123,10 +115,10 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", DisableCISkip: true, }, - version: resource.NewVersion(testPullRequests[1]), + version: resource.NewVersion(testPullRequests[1].PR), pullRequests: testPullRequests, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[0]), + resource.NewVersion(testPullRequests[0].PR), }, }, @@ -137,10 +129,10 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", IgnoreDrafts: true, }, - version: resource.NewVersion(testPullRequests[3]), + version: resource.NewVersion(testPullRequests[3].PR), pullRequests: testPullRequests, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[1]), + resource.NewVersion(testPullRequests[1].PR), }, }, @@ -151,11 +143,11 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", IgnoreDrafts: false, }, - version: resource.NewVersion(testPullRequests[3]), + version: resource.NewVersion(testPullRequests[3].PR), pullRequests: testPullRequests, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[2]), - resource.NewVersion(testPullRequests[1]), + resource.NewVersion(testPullRequests[2].PR), + resource.NewVersion(testPullRequests[1].PR), }, }, @@ -166,12 +158,12 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", DisableForks: true, }, - version: resource.NewVersion(testPullRequests[5]), + version: resource.NewVersion(testPullRequests[5].PR), pullRequests: testPullRequests, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[3]), - resource.NewVersion(testPullRequests[2]), - resource.NewVersion(testPullRequests[1]), + resource.NewVersion(testPullRequests[3].PR), + resource.NewVersion(testPullRequests[2].PR), + resource.NewVersion(testPullRequests[1].PR), }, }, @@ -184,9 +176,8 @@ func TestCheck(t *testing.T) { }, version: resource.Version{}, pullRequests: testPullRequests, - files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[6]), + resource.NewVersion(testPullRequests[6].PR), }, }, @@ -197,10 +188,10 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", RequiredReviewApprovals: 1, }, - version: resource.NewVersion(testPullRequests[8]), + version: resource.NewVersion(testPullRequests[8].PR), pullRequests: testPullRequests, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[7]), + resource.NewVersion(testPullRequests[7].PR), }, }, @@ -213,9 +204,8 @@ func TestCheck(t *testing.T) { }, version: resource.Version{}, pullRequests: testPullRequests, - files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[6]), + resource.NewVersion(testPullRequests[6].PR), }, }, @@ -228,9 +218,8 @@ func TestCheck(t *testing.T) { }, version: resource.Version{}, pullRequests: testPullRequests, - files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[9]), + resource.NewVersion(testPullRequests[9].PR), }, }, @@ -243,7 +232,6 @@ func TestCheck(t *testing.T) { }, version: resource.Version{}, pullRequests: testPullRequests[9:11], - files: [][]string{}, expected: resource.CheckResponse(nil), }, @@ -254,12 +242,11 @@ func TestCheck(t *testing.T) { AccessToken: "oauthtoken", States: []githubv4.PullRequestState{githubv4.PullRequestStateClosed, githubv4.PullRequestStateMerged}, }, - version: resource.NewVersion(testPullRequests[11]), + version: resource.NewVersion(testPullRequests[11].PR), pullRequests: testPullRequests, - files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[9]), - resource.NewVersion(testPullRequests[10]), + resource.NewVersion(testPullRequests[9].PR), + resource.NewVersion(testPullRequests[10].PR), }, }, } @@ -272,19 +259,22 @@ func TestCheck(t *testing.T) { if len(tc.source.States) > 0 { filterStates = tc.source.States } - for i := range tc.pullRequests { + var changedFilesCall int + for i, pr := range tc.pullRequests { for j := range filterStates { - if filterStates[j] == tc.pullRequests[i].PullRequestObject.State { - pullRequests = append(pullRequests, tc.pullRequests[i]) + if filterStates[j] == tc.pullRequests[i].PR.PullRequestObject.State { + pullRequests = append(pullRequests, tc.pullRequests[i].PR) break } } - } - github.ListPullRequestsReturns(pullRequests, nil) - for i, file := range tc.files { - github.ListModifiedFilesReturnsOnCall(i, file, nil) + for _, cfo := range pr.AdditionalFiles { + hasNext := len(pr.AdditionalFiles) > (i + 1) + github.GetChangedFilesReturnsOnCall(changedFilesCall, cfo, hasNext, "", nil) + changedFilesCall += 1 + } } + github.ListPullRequestsReturns(pullRequests, nil) input := resource.CheckRequest{Source: tc.source, Version: tc.version} output, err := resource.Check(input, github) @@ -352,57 +342,57 @@ func TestFilterPath(t *testing.T) { cases := []struct { description string pattern string - files []string - want []string + files []resource.ChangedFileObject + want []resource.ChangedFileObject }{ { description: "returns all matching files", pattern: "*.txt", - files: []string{ - "file1.txt", - "test/file2.txt", + files: []resource.ChangedFileObject{ + {Path: "file1.txt"}, + {Path: "test/file2.txt"}, }, - want: []string{ - "file1.txt", + want: []resource.ChangedFileObject{ + {Path: "file1.txt"}, }, }, { description: "works with wildcard", pattern: "test/*", - files: []string{ - "file1.txt", - "test/file2.txt", + files: []resource.ChangedFileObject{ + {Path: "file1.txt"}, + {Path: "test/file2.txt"}, }, - want: []string{ - "test/file2.txt", + want: []resource.ChangedFileObject{ + {Path: "test/file2.txt"}, }, }, { description: "excludes unmatched files", pattern: "*/*.txt", - files: []string{ - "test/file1.go", - "test/file2.txt", + files: []resource.ChangedFileObject{ + {Path: "test/file1.go"}, + {Path: "test/file2.txt"}, }, - want: []string{ - "test/file2.txt", + want: []resource.ChangedFileObject{ + {Path: "test/file2.txt"}, }, }, { description: "handles prefix matches", pattern: "foo/", - files: []string{ - "foo/a", - "foo/a.txt", - "foo/a/b/c/d.txt", - "foo", - "bar", - "bar/a.txt", + files: []resource.ChangedFileObject{ + {Path: "foo/a"}, + {Path: "foo/a.txt"}, + {Path: "foo/a/b/c/d.txt"}, + {Path: "foo"}, + {Path: "bar"}, + {Path: "bar/a.txt"}, }, - want: []string{ - "foo/a", - "foo/a.txt", - "foo/a/b/c/d.txt", + want: []resource.ChangedFileObject{ + {Path: "foo/a"}, + {Path: "foo/a.txt"}, + {Path: "foo/a/b/c/d.txt"}, }, }, } @@ -420,57 +410,57 @@ func TestFilterIgnorePath(t *testing.T) { cases := []struct { description string pattern string - files []string - want []string + files []resource.ChangedFileObject + want []resource.ChangedFileObject }{ { description: "excludes all matching files", pattern: "*.txt", - files: []string{ - "file1.txt", - "test/file2.txt", + files: []resource.ChangedFileObject{ + {Path: "file1.txt"}, + {Path: "test/file2.txt"}, }, - want: []string{ - "test/file2.txt", + want: []resource.ChangedFileObject{ + {Path: "test/file2.txt"}, }, }, { description: "works with wildcard", pattern: "test/*", - files: []string{ - "file1.txt", - "test/file2.txt", + files: []resource.ChangedFileObject{ + {Path: "file1.txt"}, + {Path: "test/file2.txt"}, }, - want: []string{ - "file1.txt", + want: []resource.ChangedFileObject{ + {Path: "file1.txt"}, }, }, { description: "includes unmatched files", pattern: "*/*.txt", - files: []string{ - "test/file1.go", - "test/file2.txt", + files: []resource.ChangedFileObject{ + {Path: "test/file1.go"}, + {Path: "test/file2.txt"}, }, - want: []string{ - "test/file1.go", + want: []resource.ChangedFileObject{ + {Path: "test/file1.go"}, }, }, { description: "handles prefix matches", pattern: "foo/", - files: []string{ - "foo/a", - "foo/a.txt", - "foo/a/b/c/d.txt", - "foo", - "bar", - "bar/a.txt", + files: []resource.ChangedFileObject{ + {Path: "foo/a"}, + {Path: "foo/a.txt"}, + {Path: "foo/a/b/c/d.txt"}, + {Path: "foo"}, + {Path: "bar"}, + {Path: "bar/a.txt"}, }, - want: []string{ - "foo", - "bar", - "bar/a.txt", + want: []resource.ChangedFileObject{ + {Path: "foo"}, + {Path: "bar"}, + {Path: "bar/a.txt"}, }, }, } @@ -550,3 +540,168 @@ func TestIsInsidePath(t *testing.T) { }) } } + +func TestHasWantedFiles(t *testing.T) { + cases := []struct { + description string + + paths []string + ignorePaths []string + + files [][]string + + expected bool + }{ + { + description: "true when paths in first page of files", + paths: []string{"README.md"}, + ignorePaths: []string{}, + files: [][]string{ + {"README.md"}, + }, + expected: true, + }, + { + description: "true when paths in second page of files", + paths: []string{"*.md"}, + ignorePaths: []string{}, + files: [][]string{ + {"travis.yml"}, + {"README.md"}, + }, + expected: true, + }, + { + description: "true when multiple paths but only one file matches", + paths: []string{"*.md"}, + ignorePaths: []string{}, + files: [][]string{ + {"travis.yml", "README.md"}, + }, + expected: true, + }, + { + description: "false when paths not in any page", + paths: []string{"*.md"}, + ignorePaths: []string{}, + files: [][]string{ + {"travis.yml"}, + {"travis.yml"}, + }, + expected: false, + }, + { + description: "true when paths on first page not in ignore", + paths: []string{}, + ignorePaths: []string{"*.yml"}, + files: [][]string{ + {"README.md"}, + }, + expected: true, + }, + { + description: "true when paths on second page not in ignore", + paths: []string{}, + ignorePaths: []string{"*.yml"}, + files: [][]string{ + {"travis.yml"}, + {"README.md"}, + }, + expected: true, + }, + { + description: "false when multiple ignore paths and both match", + paths: []string{}, + ignorePaths: []string{"*.md", "*.yml"}, + files: [][]string{ + {"travis.yml", "README.md"}, + }, + expected: false, + }, + { + description: "false when all pages in ignore", + paths: []string{}, + ignorePaths: []string{"*.yml"}, + files: [][]string{ + {"travis.yml"}, + {"travis2.yml"}, + }, + expected: false, + }, + { + description: "false when in both paths and ignore", + paths: []string{"*.yml"}, + ignorePaths: []string{"*.yml"}, + files: [][]string{ + {"travis.yml"}, + }, + expected: false, + }, + } + + for _, tc := range cases { + manager := new(fakes.FakeGithub) + + t.Run(tc.description, func(t *testing.T) { + var initialFiles []resource.ChangedFileObject + + for i, files := range tc.files { + if i == 0 { + // The first page of files is included in the first function call + for _, path := range files { + initialFiles = append(initialFiles, resource.ChangedFileObject{Path: path}) + } + continue + } + + // subsequent pages are retrieved from GitHub + var cfo []resource.ChangedFileObject + for _, path := range files { + cfo = append(cfo, resource.ChangedFileObject{Path: path}) + } + hasNextPage := len(tc.files) > (i + 1) + manager.GetChangedFilesReturnsOnCall(i-1, cfo, hasNextPage, "", nil) + } + + initialHasNextPage := len(tc.files) > 1 + + actual, err := resource.HasWantedFiles("foo", tc.paths, tc.ignorePaths, initialFiles, initialHasNextPage, "", manager) + assert.NoError(t, err) + assert.Equal(t, tc.expected, actual) + }) + } +} + +type CheckTestPR struct { + PR *resource.PullRequest + AdditionalFiles [][]resource.ChangedFileObject +} + +func createCheckTestPR( + count int, + baseName string, + skipCI bool, + isCrossRepo bool, + approvedReviews int, + labels []string, + isDraft bool, + state githubv4.PullRequestState, + files [][]resource.ChangedFileObject, +) *CheckTestPR { + var additionalFiles [][]resource.ChangedFileObject + + pr := createTestPR(count, baseName, skipCI, isCrossRepo, approvedReviews, labels, isDraft, state) + + // PRs retrieved by ListPullRequests - as is the case with check - include some changed files. + // We want to write tests that include changed files beyond those on the first page, however. + if len(files) > 0 { + additionalFiles = files[1:] + pr.Files = files[0] + pr.FilesPageInfo.HasNextPage = len(additionalFiles) > 0 + } + + return &CheckTestPR{ + PR: pr, + AdditionalFiles: additionalFiles, + } +} diff --git a/fakes/fake_github.go b/fakes/fake_github.go index 1847478f..a209c87a 100644 --- a/fakes/fake_github.go +++ b/fakes/fake_github.go @@ -20,19 +20,24 @@ type FakeGithub struct { deletePreviousCommentsReturnsOnCall map[int]struct { result1 error } - GetChangedFilesStub func(string, string) ([]resource.ChangedFileObject, error) + GetChangedFilesStub func(string, int, string) ([]resource.ChangedFileObject, bool, string, error) getChangedFilesMutex sync.RWMutex getChangedFilesArgsForCall []struct { arg1 string - arg2 string + arg2 int + arg3 string } getChangedFilesReturns struct { result1 []resource.ChangedFileObject - result2 error + result2 bool + result3 string + result4 error } getChangedFilesReturnsOnCall map[int]struct { result1 []resource.ChangedFileObject - result2 error + result2 bool + result3 string + result4 error } GetPullRequestStub func(string, string) (*resource.PullRequest, error) getPullRequestMutex sync.RWMutex @@ -48,19 +53,6 @@ type FakeGithub struct { result1 *resource.PullRequest result2 error } - ListModifiedFilesStub func(int) ([]string, error) - listModifiedFilesMutex sync.RWMutex - listModifiedFilesArgsForCall []struct { - arg1 int - } - listModifiedFilesReturns struct { - result1 []string - result2 error - } - listModifiedFilesReturnsOnCall map[int]struct { - result1 []string - result2 error - } ListPullRequestsStub func([]githubv4.PullRequestState) ([]*resource.PullRequest, error) listPullRequestsMutex sync.RWMutex listPullRequestsArgsForCall []struct { @@ -166,23 +158,24 @@ func (fake *FakeGithub) DeletePreviousCommentsReturnsOnCall(i int, result1 error }{result1} } -func (fake *FakeGithub) GetChangedFiles(arg1 string, arg2 string) ([]resource.ChangedFileObject, error) { +func (fake *FakeGithub) GetChangedFiles(arg1 string, arg2 int, arg3 string) ([]resource.ChangedFileObject, bool, string, error) { fake.getChangedFilesMutex.Lock() ret, specificReturn := fake.getChangedFilesReturnsOnCall[len(fake.getChangedFilesArgsForCall)] fake.getChangedFilesArgsForCall = append(fake.getChangedFilesArgsForCall, struct { arg1 string - arg2 string - }{arg1, arg2}) - fake.recordInvocation("GetChangedFiles", []interface{}{arg1, arg2}) + arg2 int + arg3 string + }{arg1, arg2, arg3}) + fake.recordInvocation("GetChangedFiles", []interface{}{arg1, arg2, arg3}) fake.getChangedFilesMutex.Unlock() if fake.GetChangedFilesStub != nil { - return fake.GetChangedFilesStub(arg1, arg2) + return fake.GetChangedFilesStub(arg1, arg2, arg3) } if specificReturn { - return ret.result1, ret.result2 + return ret.result1, ret.result2, ret.result3, ret.result4 } fakeReturns := fake.getChangedFilesReturns - return fakeReturns.result1, fakeReturns.result2 + return fakeReturns.result1, fakeReturns.result2, fakeReturns.result3, fakeReturns.result4 } func (fake *FakeGithub) GetChangedFilesCallCount() int { @@ -191,43 +184,49 @@ func (fake *FakeGithub) GetChangedFilesCallCount() int { return len(fake.getChangedFilesArgsForCall) } -func (fake *FakeGithub) GetChangedFilesCalls(stub func(string, string) ([]resource.ChangedFileObject, error)) { +func (fake *FakeGithub) GetChangedFilesCalls(stub func(string, int, string) ([]resource.ChangedFileObject, bool, string, error)) { fake.getChangedFilesMutex.Lock() defer fake.getChangedFilesMutex.Unlock() fake.GetChangedFilesStub = stub } -func (fake *FakeGithub) GetChangedFilesArgsForCall(i int) (string, string) { +func (fake *FakeGithub) GetChangedFilesArgsForCall(i int) (string, int, string) { fake.getChangedFilesMutex.RLock() defer fake.getChangedFilesMutex.RUnlock() argsForCall := fake.getChangedFilesArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 } -func (fake *FakeGithub) GetChangedFilesReturns(result1 []resource.ChangedFileObject, result2 error) { +func (fake *FakeGithub) GetChangedFilesReturns(result1 []resource.ChangedFileObject, result2 bool, result3 string, result4 error) { fake.getChangedFilesMutex.Lock() defer fake.getChangedFilesMutex.Unlock() fake.GetChangedFilesStub = nil fake.getChangedFilesReturns = struct { result1 []resource.ChangedFileObject - result2 error - }{result1, result2} + result2 bool + result3 string + result4 error + }{result1, result2, result3, result4} } -func (fake *FakeGithub) GetChangedFilesReturnsOnCall(i int, result1 []resource.ChangedFileObject, result2 error) { +func (fake *FakeGithub) GetChangedFilesReturnsOnCall(i int, result1 []resource.ChangedFileObject, result2 bool, result3 string, result4 error) { fake.getChangedFilesMutex.Lock() defer fake.getChangedFilesMutex.Unlock() fake.GetChangedFilesStub = nil if fake.getChangedFilesReturnsOnCall == nil { fake.getChangedFilesReturnsOnCall = make(map[int]struct { result1 []resource.ChangedFileObject - result2 error + result2 bool + result3 string + result4 error }) } fake.getChangedFilesReturnsOnCall[i] = struct { result1 []resource.ChangedFileObject - result2 error - }{result1, result2} + result2 bool + result3 string + result4 error + }{result1, result2, result3, result4} } func (fake *FakeGithub) GetPullRequest(arg1 string, arg2 string) (*resource.PullRequest, error) { @@ -294,69 +293,6 @@ func (fake *FakeGithub) GetPullRequestReturnsOnCall(i int, result1 *resource.Pul }{result1, result2} } -func (fake *FakeGithub) ListModifiedFiles(arg1 int) ([]string, error) { - fake.listModifiedFilesMutex.Lock() - ret, specificReturn := fake.listModifiedFilesReturnsOnCall[len(fake.listModifiedFilesArgsForCall)] - fake.listModifiedFilesArgsForCall = append(fake.listModifiedFilesArgsForCall, struct { - arg1 int - }{arg1}) - fake.recordInvocation("ListModifiedFiles", []interface{}{arg1}) - fake.listModifiedFilesMutex.Unlock() - if fake.ListModifiedFilesStub != nil { - return fake.ListModifiedFilesStub(arg1) - } - if specificReturn { - return ret.result1, ret.result2 - } - fakeReturns := fake.listModifiedFilesReturns - return fakeReturns.result1, fakeReturns.result2 -} - -func (fake *FakeGithub) ListModifiedFilesCallCount() int { - fake.listModifiedFilesMutex.RLock() - defer fake.listModifiedFilesMutex.RUnlock() - return len(fake.listModifiedFilesArgsForCall) -} - -func (fake *FakeGithub) ListModifiedFilesCalls(stub func(int) ([]string, error)) { - fake.listModifiedFilesMutex.Lock() - defer fake.listModifiedFilesMutex.Unlock() - fake.ListModifiedFilesStub = stub -} - -func (fake *FakeGithub) ListModifiedFilesArgsForCall(i int) int { - fake.listModifiedFilesMutex.RLock() - defer fake.listModifiedFilesMutex.RUnlock() - argsForCall := fake.listModifiedFilesArgsForCall[i] - return argsForCall.arg1 -} - -func (fake *FakeGithub) ListModifiedFilesReturns(result1 []string, result2 error) { - fake.listModifiedFilesMutex.Lock() - defer fake.listModifiedFilesMutex.Unlock() - fake.ListModifiedFilesStub = nil - fake.listModifiedFilesReturns = struct { - result1 []string - result2 error - }{result1, result2} -} - -func (fake *FakeGithub) ListModifiedFilesReturnsOnCall(i int, result1 []string, result2 error) { - fake.listModifiedFilesMutex.Lock() - defer fake.listModifiedFilesMutex.Unlock() - fake.ListModifiedFilesStub = nil - if fake.listModifiedFilesReturnsOnCall == nil { - fake.listModifiedFilesReturnsOnCall = make(map[int]struct { - result1 []string - result2 error - }) - } - fake.listModifiedFilesReturnsOnCall[i] = struct { - result1 []string - result2 error - }{result1, result2} -} - func (fake *FakeGithub) ListPullRequests(arg1 []githubv4.PullRequestState) ([]*resource.PullRequest, error) { var arg1Copy []githubv4.PullRequestState if arg1 != nil { @@ -560,8 +496,6 @@ func (fake *FakeGithub) Invocations() map[string][][]interface{} { defer fake.getChangedFilesMutex.RUnlock() fake.getPullRequestMutex.RLock() defer fake.getPullRequestMutex.RUnlock() - fake.listModifiedFilesMutex.RLock() - defer fake.listModifiedFilesMutex.RUnlock() fake.listPullRequestsMutex.RLock() defer fake.listPullRequestsMutex.RUnlock() fake.postCommentMutex.RLock() diff --git a/github.go b/github.go index ab10cbdc..de91f51f 100644 --- a/github.go +++ b/github.go @@ -21,10 +21,9 @@ import ( //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o fakes/fake_github.go . Github type Github interface { ListPullRequests([]githubv4.PullRequestState) ([]*PullRequest, error) - ListModifiedFiles(int) ([]string, error) PostComment(string, string) error GetPullRequest(string, string) (*PullRequest, error) - GetChangedFiles(string, string) ([]ChangedFileObject, error) + GetChangedFiles(string, int, string) ([]ChangedFileObject, bool, string, error) UpdateCommitStatus(string, string, string, string, string, string) error DeletePreviousComments(string) error } @@ -122,6 +121,14 @@ func (m *GithubClient) ListPullRequests(prStates []githubv4.PullRequestState) ([ } } } `graphql:"labels(first:$labelsFirst)"` + Files struct { + Edges []struct { + Node struct { + ChangedFileObject + } + } `graphql:"edges"` + PageInfo PageInfoObject `graphql:"pageInfo"` + } `graphql:"files(first:$changedFilesFirst)"` } } PageInfo struct { @@ -133,14 +140,15 @@ func (m *GithubClient) ListPullRequests(prStates []githubv4.PullRequestState) ([ } vars := map[string]interface{}{ - "repositoryOwner": githubv4.String(m.Owner), - "repositoryName": githubv4.String(m.Repository), - "prFirst": githubv4.Int(100), - "prStates": prStates, - "prCursor": (*githubv4.String)(nil), - "commitsLast": githubv4.Int(1), - "prReviewStates": []githubv4.PullRequestReviewState{githubv4.PullRequestReviewStateApproved}, - "labelsFirst": githubv4.Int(100), + "repositoryOwner": githubv4.String(m.Owner), + "repositoryName": githubv4.String(m.Repository), + "prFirst": githubv4.Int(100), + "prStates": prStates, + "prCursor": (*githubv4.String)(nil), + "commitsLast": githubv4.Int(1), + "prReviewStates": []githubv4.PullRequestReviewState{githubv4.PullRequestReviewStateApproved}, + "labelsFirst": githubv4.Int(100), + "changedFilesFirst": githubv4.Int(100), } var response []*PullRequest @@ -154,12 +162,19 @@ func (m *GithubClient) ListPullRequests(prStates []githubv4.PullRequestState) ([ labels = append(labels, l.Node.LabelObject) } + files := make([]ChangedFileObject, len(p.Node.Files.Edges)) + for _, f := range p.Node.Files.Edges { + files = append(files, f.Node.ChangedFileObject) + } + for _, c := range p.Node.Commits.Edges { response = append(response, &PullRequest{ PullRequestObject: p.Node.PullRequestObject, Tip: c.Node.Commit, ApprovedReviewCount: p.Node.Reviews.TotalCount, Labels: labels, + Files: files, + FilesPageInfo: p.Node.Files.PageInfo, }) } } @@ -171,35 +186,6 @@ func (m *GithubClient) ListPullRequests(prStates []githubv4.PullRequestState) ([ return response, nil } -// ListModifiedFiles in a pull request (not supported by V4 API). -func (m *GithubClient) ListModifiedFiles(prNumber int) ([]string, error) { - var files []string - - opt := &github.ListOptions{ - PerPage: 100, - } - for { - result, response, err := m.V3.PullRequests.ListFiles( - context.TODO(), - m.Owner, - m.Repository, - prNumber, - opt, - ) - if err != nil { - return nil, err - } - for _, f := range result { - files = append(files, *f.Filename) - } - if response.NextPage == 0 { - break - } - opt.Page = response.NextPage - } - return files, nil -} - // PostComment to a pull request or issue. func (m *GithubClient) PostComment(prNumber, comment string) error { pr, err := strconv.Atoi(prNumber) @@ -220,10 +206,10 @@ func (m *GithubClient) PostComment(prNumber, comment string) error { } // GetChangedFiles ... -func (m *GithubClient) GetChangedFiles(prNumber string, commitRef string) ([]ChangedFileObject, error) { +func (m *GithubClient) GetChangedFiles(prNumber string, perPage int, afterCursor string) ([]ChangedFileObject, bool, string, error) { pr, err := strconv.Atoi(prNumber) if err != nil { - return nil, fmt.Errorf("failed to convert pull request number to int: %s", err) + return nil, false, "", fmt.Errorf("failed to convert pull request number to int: %s", err) } var cfo []ChangedFileObject @@ -246,33 +232,26 @@ func (m *GithubClient) GetChangedFiles(prNumber string, commitRef string) ([]Cha } `graphql:"repository(owner:$repositoryOwner,name:$repositoryName)"` } - offset := "" - - for { - vars := map[string]interface{}{ - "repositoryOwner": githubv4.String(m.Owner), - "repositoryName": githubv4.String(m.Repository), - "prNumber": githubv4.Int(pr), - "changedFilesFirst": githubv4.Int(100), - "changedFilesEndCursor": githubv4.String(offset), - } - - if err := m.V4.Query(context.TODO(), &filequery, vars); err != nil { - return nil, err - } - - for _, f := range filequery.Repository.PullRequest.Files.Edges { - cfo = append(cfo, ChangedFileObject{Path: f.Node.Path}) - } + vars := map[string]interface{}{ + "repositoryOwner": githubv4.String(m.Owner), + "repositoryName": githubv4.String(m.Repository), + "prNumber": githubv4.Int(pr), + "changedFilesFirst": githubv4.Int(perPage), + "changedFilesEndCursor": githubv4.String(afterCursor), + } - if !filequery.Repository.PullRequest.Files.PageInfo.HasNextPage { - break - } + if err := m.V4.Query(context.TODO(), &filequery, vars); err != nil { + return nil, false, "", err + } - offset = string(filequery.Repository.PullRequest.Files.PageInfo.EndCursor) + for _, f := range filequery.Repository.PullRequest.Files.Edges { + cfo = append(cfo, ChangedFileObject{Path: f.Node.Path}) } - return cfo, nil + hasNext := filequery.Repository.PullRequest.Files.PageInfo.HasNextPage + nextCursor := string(filequery.Repository.PullRequest.Files.PageInfo.EndCursor) + + return cfo, hasNext, nextCursor, nil } // GetPullRequest ... diff --git a/in.go b/in.go index fa0eb242..d30325d0 100644 --- a/in.go +++ b/in.go @@ -105,15 +105,24 @@ func Get(request GetRequest, github Github, git Git, outputDir string) (*GetResp } if request.Params.ListChangedFiles { - cfol, err := github.GetChangedFiles(request.Version.PR, request.Version.Commit) - if err != nil { - return nil, fmt.Errorf("failed to fetch list of changed files: %s", err) - } - + var cfol []ChangedFileObject var fl []byte - - for _, v := range cfol { - fl = append(fl, []byte(v.Path+"\n")...) + var hasNext bool + var nextCursor string + + for { + cfol, hasNext, nextCursor, err = github.GetChangedFiles(request.Version.PR, 100, nextCursor) + if err != nil { + return nil, fmt.Errorf("failed to fetch list of changed files: %s", err) + } + + for _, v := range cfol { + fl = append(fl, []byte(v.Path+"\n")...) + } + + if !hasNext { + break + } } // Create List with changed files diff --git a/in_test.go b/in_test.go index 17e7abfc..f905cbf5 100644 --- a/in_test.go +++ b/in_test.go @@ -162,7 +162,7 @@ func TestGet(t *testing.T) { github.GetPullRequestReturns(tc.pullRequest, nil) if tc.files != nil { - github.GetChangedFilesReturns(tc.files, nil) + github.GetChangedFilesReturns(tc.files, false, "", nil) } git := new(fakes.FakeGit) diff --git a/models.go b/models.go index 9e4e7b1c..c0a57d22 100644 --- a/models.go +++ b/models.go @@ -95,6 +95,8 @@ type PullRequest struct { Tip CommitObject ApprovedReviewCount int Labels []LabelObject + Files []ChangedFileObject + FilesPageInfo PageInfoObject } // PullRequestObject represents the GraphQL commit node. @@ -155,3 +157,8 @@ type ChangedFileObject struct { type LabelObject struct { Name string } + +type PageInfoObject struct { + EndCursor githubv4.String + HasNextPage bool +}