From 4f3ff8d88e7e7b0ab8ab3a33c7ae81334b6200c7 Mon Sep 17 00:00:00 2001 From: Charles Treatman Date: Thu, 6 May 2021 14:44:10 -0400 Subject: [PATCH] Close #26: Filter PRs by updated date instead of commit date This supersedes #205. This resource can inadvertently miss Pull Requests due to out-of-order commits across PRs. If PR#2 is opened after PR#1, but the head commit of PR#2 is older than the head commit of PR#1, the resource will not include PR#2 in the list of new versions provided to Concourse. In #205, I removed the date filter entirely. This ensures that the PR resource will find all PRs that match the explicitly-configured filters. While Concourse can detect and ignore duplicate versions, it has to run a database query for every version returned by a `check`, so removing the date filter entirely would increase load on a Concourse database. (That said, I'm not sure whether this increased load is a particular concern, and other resources don't seem to make much effort to avoid returning duplicate versions from a `check`.) To avoid that extra load on a Concourse database, this change instead replaces the filter by commit date in `check.go` with a filter by updated date in the GraphQL query to list pull requests. This should reduce the number of duplicate versions returned by a `check` while still allowing the PR resource to detect PRs with out-of-order head commits. --- check.go | 7 +---- check_test.go | 53 ++++++++++++++++++++++--------- e2e/e2e_test.go | 63 ++++++++++++++++++++----------------- fakes/fake_github.go | 19 +++++++----- github.go | 74 +++++++++++++++++++++++++------------------- 5 files changed, 127 insertions(+), 89 deletions(-) diff --git a/check.go b/check.go index 64df9e24..6407caa8 100644 --- a/check.go +++ b/check.go @@ -20,7 +20,7 @@ func Check(request CheckRequest, manager Github) (CheckResponse, error) { filterStates = request.Source.States } - pulls, err := manager.ListPullRequests(filterStates) + pulls, err := manager.ListPullRequests(filterStates, request.Version.CommittedDate) if err != nil { return nil, fmt.Errorf("failed to get last commits: %s", err) } @@ -44,11 +44,6 @@ Loop: continue } - // Filter out commits that are too old. - if !p.UpdatedDate().Time.After(request.Version.CommittedDate) { - continue - } - // Filter out pull request if it does not contain at least one of the desired labels if len(request.Source.Labels) > 0 { labelFound := false diff --git a/check_test.go b/check_test.go index 8c422914..956a8728 100644 --- a/check_test.go +++ b/check_test.go @@ -50,7 +50,7 @@ func TestCheck(t *testing.T) { }, { - description: "check returns the previous version when its still latest", + description: "check returns all open PRs if there is a previous", source: resource.Source{ Repository: "itsdalmo/test-repository", AccessToken: "oauthtoken", @@ -59,20 +59,13 @@ func TestCheck(t *testing.T) { pullRequests: testPullRequests, files: [][]string{}, expected: resource.CheckResponse{ - resource.NewVersion(testPullRequests[1]), - }, - }, - - { - description: "check returns all new versions since the last", - source: resource.Source{ - Repository: "itsdalmo/test-repository", - AccessToken: "oauthtoken", - }, - version: resource.NewVersion(testPullRequests[3]), - pullRequests: testPullRequests, - files: [][]string{}, - expected: resource.CheckResponse{ + resource.NewVersion(testPullRequests[11]), + resource.NewVersion(testPullRequests[8]), + resource.NewVersion(testPullRequests[7]), + resource.NewVersion(testPullRequests[6]), + resource.NewVersion(testPullRequests[5]), + resource.NewVersion(testPullRequests[4]), + resource.NewVersion(testPullRequests[3]), resource.NewVersion(testPullRequests[2]), resource.NewVersion(testPullRequests[1]), }, @@ -93,6 +86,7 @@ func TestCheck(t *testing.T) { {"terraform/modules/variables.tf", "travis.yml"}, }, expected: resource.CheckResponse{ + resource.NewVersion(testPullRequests[3]), resource.NewVersion(testPullRequests[2]), }, }, @@ -112,6 +106,7 @@ func TestCheck(t *testing.T) { {"terraform/modules/variables.tf", "travis.yml"}, }, expected: resource.CheckResponse{ + resource.NewVersion(testPullRequests[3]), resource.NewVersion(testPullRequests[2]), }, }, @@ -126,6 +121,15 @@ func TestCheck(t *testing.T) { version: resource.NewVersion(testPullRequests[1]), pullRequests: testPullRequests, expected: resource.CheckResponse{ + resource.NewVersion(testPullRequests[11]), + resource.NewVersion(testPullRequests[8]), + resource.NewVersion(testPullRequests[7]), + resource.NewVersion(testPullRequests[6]), + resource.NewVersion(testPullRequests[5]), + resource.NewVersion(testPullRequests[4]), + resource.NewVersion(testPullRequests[3]), + resource.NewVersion(testPullRequests[2]), + resource.NewVersion(testPullRequests[1]), resource.NewVersion(testPullRequests[0]), }, }, @@ -140,6 +144,13 @@ func TestCheck(t *testing.T) { version: resource.NewVersion(testPullRequests[3]), pullRequests: testPullRequests, expected: resource.CheckResponse{ + resource.NewVersion(testPullRequests[11]), + resource.NewVersion(testPullRequests[8]), + resource.NewVersion(testPullRequests[7]), + resource.NewVersion(testPullRequests[6]), + resource.NewVersion(testPullRequests[5]), + resource.NewVersion(testPullRequests[4]), + resource.NewVersion(testPullRequests[3]), resource.NewVersion(testPullRequests[1]), }, }, @@ -154,6 +165,13 @@ func TestCheck(t *testing.T) { version: resource.NewVersion(testPullRequests[3]), pullRequests: testPullRequests, expected: resource.CheckResponse{ + resource.NewVersion(testPullRequests[11]), + resource.NewVersion(testPullRequests[8]), + resource.NewVersion(testPullRequests[7]), + resource.NewVersion(testPullRequests[6]), + resource.NewVersion(testPullRequests[5]), + resource.NewVersion(testPullRequests[4]), + resource.NewVersion(testPullRequests[3]), resource.NewVersion(testPullRequests[2]), resource.NewVersion(testPullRequests[1]), }, @@ -169,6 +187,11 @@ func TestCheck(t *testing.T) { version: resource.NewVersion(testPullRequests[5]), pullRequests: testPullRequests, expected: resource.CheckResponse{ + resource.NewVersion(testPullRequests[11]), + resource.NewVersion(testPullRequests[8]), + resource.NewVersion(testPullRequests[7]), + resource.NewVersion(testPullRequests[6]), + resource.NewVersion(testPullRequests[5]), resource.NewVersion(testPullRequests[3]), resource.NewVersion(testPullRequests[2]), resource.NewVersion(testPullRequests[1]), diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 523838c2..ee1bcd86 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -23,6 +23,9 @@ import ( ) var ( + firstCommitID = "23dc9f552bf989d1a4aeb65ce23351dee0ec9019" + firstPullRequestID = "3" + firstDateTime = time.Date(2018, time.May, 11, 7, 28, 56, 0, time.UTC) targetCommitID = "a5114f6ab89f4b736655642a11e8d15ce363d882" targetPullRequestID = "4" targetDateTime = time.Date(2018, time.May, 11, 8, 43, 48, 0, time.UTC) @@ -49,19 +52,20 @@ func TestCheckE2E(t *testing.T) { }, version: resource.Version{}, expected: resource.CheckResponse{ - resource.Version{PR: latestPullRequestID, Commit: latestCommitID, CommittedDate: latestDateTime}, + resource.Version{PR: latestPullRequestID, Commit: latestCommitID, CommittedDate: latestDateTime, ApprovedReviewCount: "0", State: "OPEN"}, }, }, { - description: "check returns the previous version when its still latest", + description: "check returns all open PRs if there is a previous version", source: resource.Source{ Repository: "itsdalmo/test-repository", AccessToken: os.Getenv("GITHUB_ACCESS_TOKEN"), }, version: resource.Version{PR: latestPullRequestID, Commit: latestCommitID, CommittedDate: latestDateTime}, expected: resource.CheckResponse{ - resource.Version{PR: latestPullRequestID, Commit: latestCommitID, CommittedDate: latestDateTime}, + resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime, ApprovedReviewCount: "1", State: "OPEN"}, + resource.Version{PR: latestPullRequestID, Commit: latestCommitID, CommittedDate: latestDateTime, ApprovedReviewCount: "0", State: "OPEN"}, }, }, @@ -73,7 +77,8 @@ func TestCheckE2E(t *testing.T) { }, version: resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime}, expected: resource.CheckResponse{ - resource.Version{PR: latestPullRequestID, Commit: latestCommitID, CommittedDate: latestDateTime}, + resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime, ApprovedReviewCount: "1", State: "OPEN"}, + resource.Version{PR: latestPullRequestID, Commit: latestCommitID, CommittedDate: latestDateTime, ApprovedReviewCount: "0", State: "OPEN"}, }, }, @@ -86,7 +91,7 @@ func TestCheckE2E(t *testing.T) { }, version: resource.Version{}, expected: resource.CheckResponse{ - resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime}, + resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime, ApprovedReviewCount: "1", State: "OPEN"}, }, }, @@ -99,7 +104,7 @@ func TestCheckE2E(t *testing.T) { }, version: resource.Version{}, expected: resource.CheckResponse{ - resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime}, + resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime, ApprovedReviewCount: "1", State: "OPEN"}, }, }, @@ -113,7 +118,7 @@ func TestCheckE2E(t *testing.T) { }, version: resource.Version{}, expected: resource.CheckResponse{ - resource.Version{PR: latestPullRequestID, Commit: latestCommitID, CommittedDate: latestDateTime}, + resource.Version{PR: latestPullRequestID, Commit: latestCommitID, CommittedDate: latestDateTime, ApprovedReviewCount: "0", State: "OPEN"}, }, }, @@ -129,7 +134,7 @@ func TestCheckE2E(t *testing.T) { }, version: resource.Version{}, expected: resource.CheckResponse{ - resource.Version{PR: developPullRequestID, Commit: developCommitID, CommittedDate: developDateTime}, + resource.Version{PR: developPullRequestID, Commit: developCommitID, CommittedDate: developDateTime, ApprovedReviewCount: "0", State: "OPEN"}, }, }, @@ -144,7 +149,7 @@ func TestCheckE2E(t *testing.T) { }, version: resource.Version{}, expected: resource.CheckResponse{ - resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime}, + resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime, ApprovedReviewCount: "1", State: "OPEN"}, }, }, @@ -170,7 +175,7 @@ func TestCheckE2E(t *testing.T) { }, version: resource.Version{}, expected: resource.CheckResponse{ - resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime}, + resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime, ApprovedReviewCount: "1", State: "OPEN"}, }, }, } @@ -204,7 +209,7 @@ func TestCheckAPICostE2E(t *testing.T) { AccessToken: os.Getenv("GITHUB_ACCESS_TOKEN"), }, version: resource.Version{}, - expected: 2, + expected: 1, }, } @@ -254,8 +259,8 @@ func TestGetAndPutE2E(t *testing.T) { }, getParameters: resource.GetParameters{}, putParameters: resource.PutParameters{}, - versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z"}`, - metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"}]`, + versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z","approved_review_count":"","state":""}`, + metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"},{"name":"state","value":"OPEN"}]`, metadataFiles: map[string]string{ "pr": "4", "url": "https://github.com/itsdalmo/test-repository/pull/4", @@ -267,7 +272,7 @@ func TestGetAndPutE2E(t *testing.T) { "author": "itsdalmo", }, expectedCommitCount: 10, - expectedCommits: []string{"Merge commit 'a5114f6ab89f4b736655642a11e8d15ce363d882'"}, + expectedCommits: []string{"Merge commit 'a5114f6ab89f4b736655642a11e8d15ce363d882' into master"}, }, { description: "get works when rebasing", @@ -286,8 +291,8 @@ func TestGetAndPutE2E(t *testing.T) { IntegrationTool: "rebase", }, putParameters: resource.PutParameters{}, - versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z"}`, - metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"}]`, + versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z","approved_review_count":"","state":""}`, + metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"},{"name":"state","value":"OPEN"}]`, expectedCommitCount: 9, expectedCommits: []string{"Push 2."}, }, @@ -308,8 +313,8 @@ func TestGetAndPutE2E(t *testing.T) { IntegrationTool: "checkout", }, putParameters: resource.PutParameters{}, - versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z"}`, - metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"}]`, + versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z","approved_review_count":"","state":""}`, + metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"},{"name":"state","value":"OPEN"}]`, expectedCommitCount: 7, expectedCommits: []string{ "Push 2.", @@ -336,8 +341,8 @@ func TestGetAndPutE2E(t *testing.T) { }, getParameters: resource.GetParameters{}, putParameters: resource.PutParameters{}, - versionString: `{"pr":"6","commit":"ac771f3b69cbd63b22bbda553f827ab36150c640","committed":"0001-01-01T00:00:00Z"}`, - metadataString: `[{"name":"pr","value":"6"},{"name":"title","value":"[skip ci] Add a PR with a non-master base"},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/6"},{"name":"head_name","value":"test-develop-pr"},{"name":"head_sha","value":"ac771f3b69cbd63b22bbda553f827ab36150c640"},{"name":"base_name","value":"develop"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"[skip ci] Add a PR with a non-master base"},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"}]`, + versionString: `{"pr":"6","commit":"ac771f3b69cbd63b22bbda553f827ab36150c640","committed":"0001-01-01T00:00:00Z","approved_review_count":"","state":""}`, + metadataString: `[{"name":"pr","value":"6"},{"name":"title","value":"[skip ci] Add a PR with a non-master base"},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/6"},{"name":"head_name","value":"test-develop-pr"},{"name":"head_sha","value":"ac771f3b69cbd63b22bbda553f827ab36150c640"},{"name":"base_name","value":"develop"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"[skip ci] Add a PR with a non-master base"},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"},{"name":"state","value":"OPEN"}]`, expectedCommitCount: 5, expectedCommits: []string{"[skip ci] Add a PR with a non-master base"}, // This merge ends up being fast-forwarded }, @@ -357,10 +362,10 @@ func TestGetAndPutE2E(t *testing.T) { }, getParameters: resource.GetParameters{}, putParameters: resource.PutParameters{}, - versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z"}`, - metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"}]`, + versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z","approved_review_count":"","state":""}`, + metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"},{"name":"state","value":"OPEN"}]`, expectedCommitCount: 10, - expectedCommits: []string{"Merge commit 'a5114f6ab89f4b736655642a11e8d15ce363d882'"}, + expectedCommits: []string{"Merge commit 'a5114f6ab89f4b736655642a11e8d15ce363d882' into master"}, }, { description: "get works with git_depth", @@ -375,11 +380,11 @@ func TestGetAndPutE2E(t *testing.T) { }, getParameters: resource.GetParameters{GitDepth: 6}, putParameters: resource.PutParameters{}, - versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z"}`, - metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"}]`, + versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z","approved_review_count":"","state":""}`, + metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"},{"name":"state","value":"OPEN"}]`, expectedCommitCount: 9, expectedCommits: []string{ - "Merge commit 'a5114f6ab89f4b736655642a11e8d15ce363d882'", + "Merge commit 'a5114f6ab89f4b736655642a11e8d15ce363d882' into master", "Push 2.", "Push 1.", "Add another commit to the 2nd PR to verify concourse behaviour.", @@ -405,11 +410,11 @@ func TestGetAndPutE2E(t *testing.T) { ListChangedFiles: true, }, putParameters: resource.PutParameters{}, - versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z"}`, - metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"}]`, + versionString: `{"pr":"4","commit":"a5114f6ab89f4b736655642a11e8d15ce363d882","committed":"0001-01-01T00:00:00Z","approved_review_count":"","state":""}`, + metadataString: `[{"name":"pr","value":"4"},{"name":"title","value":"Add comment from 2nd pull request."},{"name":"url","value":"https://github.com/itsdalmo/test-repository/pull/4"},{"name":"head_name","value":"my_second_pull"},{"name":"head_sha","value":"a5114f6ab89f4b736655642a11e8d15ce363d882"},{"name":"base_name","value":"master"},{"name":"base_sha","value":"93eeeedb8a16e6662062d1eca5655108977cc59a"},{"name":"message","value":"Push 2."},{"name":"author","value":"itsdalmo"},{"name":"author_email","value":"kristian@doingit.no"},{"name":"state","value":"OPEN"}]`, filesString: "README.md\ntest.txt\n", expectedCommitCount: 10, - expectedCommits: []string{"Merge commit 'a5114f6ab89f4b736655642a11e8d15ce363d882'"}, + expectedCommits: []string{"Merge commit 'a5114f6ab89f4b736655642a11e8d15ce363d882' into master"}, }, } diff --git a/fakes/fake_github.go b/fakes/fake_github.go index 1847478f..151890bf 100644 --- a/fakes/fake_github.go +++ b/fakes/fake_github.go @@ -3,6 +3,7 @@ package fakes import ( "sync" + "time" "github.com/shurcooL/githubv4" resource "github.com/telia-oss/github-pr-resource" @@ -61,10 +62,11 @@ type FakeGithub struct { result1 []string result2 error } - ListPullRequestsStub func([]githubv4.PullRequestState) ([]*resource.PullRequest, error) + ListPullRequestsStub func([]githubv4.PullRequestState, time.Time) ([]*resource.PullRequest, error) listPullRequestsMutex sync.RWMutex listPullRequestsArgsForCall []struct { arg1 []githubv4.PullRequestState + arg2 time.Time } listPullRequestsReturns struct { result1 []*resource.PullRequest @@ -357,7 +359,7 @@ func (fake *FakeGithub) ListModifiedFilesReturnsOnCall(i int, result1 []string, }{result1, result2} } -func (fake *FakeGithub) ListPullRequests(arg1 []githubv4.PullRequestState) ([]*resource.PullRequest, error) { +func (fake *FakeGithub) ListPullRequests(arg1 []githubv4.PullRequestState, arg2 time.Time) ([]*resource.PullRequest, error) { var arg1Copy []githubv4.PullRequestState if arg1 != nil { arg1Copy = make([]githubv4.PullRequestState, len(arg1)) @@ -367,11 +369,12 @@ func (fake *FakeGithub) ListPullRequests(arg1 []githubv4.PullRequestState) ([]*r ret, specificReturn := fake.listPullRequestsReturnsOnCall[len(fake.listPullRequestsArgsForCall)] fake.listPullRequestsArgsForCall = append(fake.listPullRequestsArgsForCall, struct { arg1 []githubv4.PullRequestState - }{arg1Copy}) - fake.recordInvocation("ListPullRequests", []interface{}{arg1Copy}) + arg2 time.Time + }{arg1Copy, arg2}) + fake.recordInvocation("ListPullRequests", []interface{}{arg1Copy, arg2}) fake.listPullRequestsMutex.Unlock() if fake.ListPullRequestsStub != nil { - return fake.ListPullRequestsStub(arg1) + return fake.ListPullRequestsStub(arg1, arg2) } if specificReturn { return ret.result1, ret.result2 @@ -386,17 +389,17 @@ func (fake *FakeGithub) ListPullRequestsCallCount() int { return len(fake.listPullRequestsArgsForCall) } -func (fake *FakeGithub) ListPullRequestsCalls(stub func([]githubv4.PullRequestState) ([]*resource.PullRequest, error)) { +func (fake *FakeGithub) ListPullRequestsCalls(stub func([]githubv4.PullRequestState, time.Time) ([]*resource.PullRequest, error)) { fake.listPullRequestsMutex.Lock() defer fake.listPullRequestsMutex.Unlock() fake.ListPullRequestsStub = stub } -func (fake *FakeGithub) ListPullRequestsArgsForCall(i int) []githubv4.PullRequestState { +func (fake *FakeGithub) ListPullRequestsArgsForCall(i int) ([]githubv4.PullRequestState, time.Time) { fake.listPullRequestsMutex.RLock() defer fake.listPullRequestsMutex.RUnlock() argsForCall := fake.listPullRequestsArgsForCall[i] - return argsForCall.arg1 + return argsForCall.arg1, argsForCall.arg2 } func (fake *FakeGithub) ListPullRequestsReturns(result1 []*resource.PullRequest, result2 error) { diff --git a/github.go b/github.go index ab10cbdc..390e81dd 100644 --- a/github.go +++ b/github.go @@ -11,6 +11,7 @@ import ( "path" "strconv" "strings" + "time" "github.com/google/go-github/v28/github" "github.com/shurcooL/githubv4" @@ -20,7 +21,7 @@ import ( // Github for testing purposes. //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -o fakes/fake_github.go . Github type Github interface { - ListPullRequests([]githubv4.PullRequestState) ([]*PullRequest, error) + ListPullRequests([]githubv4.PullRequestState, time.Time) ([]*PullRequest, error) ListModifiedFiles(int) ([]string, error) PostComment(string, string) error GetPullRequest(string, string) (*PullRequest, error) @@ -98,16 +99,18 @@ func NewGithubClient(s *Source) (*GithubClient, error) { } // ListPullRequests gets the last commit on all pull requests with the matching state. -func (m *GithubClient) ListPullRequests(prStates []githubv4.PullRequestState) ([]*PullRequest, error) { - var query struct { - Repository struct { - PullRequests struct { - Edges []struct { - Node struct { +func (m *GithubClient) ListPullRequests(prStates []githubv4.PullRequestState, since time.Time) ([]*PullRequest, error) { + var prSearch struct { + Search struct { + Edges []struct { + Node struct { + PullRequest struct { PullRequestObject + Reviews struct { TotalCount int } `graphql:"reviews(states: $prReviewStates)"` + Commits struct { Edges []struct { Node struct { @@ -115,6 +118,7 @@ func (m *GithubClient) ListPullRequests(prStates []githubv4.PullRequestState) ([ } } } `graphql:"commits(last:$commitsLast)"` + Labels struct { Edges []struct { Node struct { @@ -122,51 +126,59 @@ func (m *GithubClient) ListPullRequests(prStates []githubv4.PullRequestState) ([ } } } `graphql:"labels(first:$labelsFirst)"` - } - } - PageInfo struct { - EndCursor githubv4.String - HasNextPage bool + } `graphql:"... on PullRequest"` } - } `graphql:"pullRequests(first:$prFirst,states:$prStates,after:$prCursor)"` - } `graphql:"repository(owner:$repositoryOwner,name:$repositoryName)"` + } + PageInfo struct { + EndCursor githubv4.String + HasNextPage bool + } + } `graphql:"search(query:$query,type:ISSUE,last:$prFirst,after:$prCursor)"` + } + + query := fmt.Sprintf("is:pr repo:%s/%s", m.Owner, m.Repository) + + for _, state := range prStates { + query += strings.ToLower(fmt.Sprintf(" state:%s", state)) + } + + if !since.IsZero() { + query += fmt.Sprintf(" updated:>=%s", since.Format(time.RFC3339)) } 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), + "prFirst": githubv4.Int(100), + "prCursor": (*githubv4.String)(nil), + "prReviewStates": []githubv4.PullRequestReviewState{githubv4.PullRequestReviewStateApproved}, + "commitsLast": githubv4.Int(1), + "labelsFirst": githubv4.Int(100), + "query": githubv4.String(query), } var response []*PullRequest for { - if err := m.V4.Query(context.TODO(), &query, vars); err != nil { + if err := m.V4.Query(context.TODO(), &prSearch, vars); err != nil { return nil, err } - for _, p := range query.Repository.PullRequests.Edges { - labels := make([]LabelObject, len(p.Node.Labels.Edges)) - for _, l := range p.Node.Labels.Edges { + for _, p := range prSearch.Search.Edges { + labels := make([]LabelObject, len(p.Node.PullRequest.Labels.Edges)) + for _, l := range p.Node.PullRequest.Labels.Edges { labels = append(labels, l.Node.LabelObject) } - for _, c := range p.Node.Commits.Edges { + for _, c := range p.Node.PullRequest.Commits.Edges { response = append(response, &PullRequest{ - PullRequestObject: p.Node.PullRequestObject, + PullRequestObject: p.Node.PullRequest.PullRequestObject, Tip: c.Node.Commit, - ApprovedReviewCount: p.Node.Reviews.TotalCount, + ApprovedReviewCount: p.Node.PullRequest.Reviews.TotalCount, Labels: labels, }) } } - if !query.Repository.PullRequests.PageInfo.HasNextPage { + if !prSearch.Search.PageInfo.HasNextPage { break } - vars["prCursor"] = query.Repository.PullRequests.PageInfo.EndCursor + vars["prCursor"] = prSearch.Search.PageInfo.EndCursor } return response, nil }