Close #26: Return all open PRs instead of filtering by date

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. Rather than attempt to find a different way of tracking which PRs are "new" given an input version, we can remove the date-based filtering and return all open PRs. Concourse can deduplicate versions based on metadata, which means that we will only trigger new jobs for versions that Concourse hasn't seen before. This makes it easier for teams to use this resource to track PRs in Concourse, since they no longer have to ensure that a PR has a later head commit than all currently-opened PRs in order to notify Concourse that their PR exists.
telia-oss · Feb 9, 2021 · b2493fb · b2493fb
1 parent 9ec47e2
commit b2493fb
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 33 deletions.
diff --git a/check.go b/check.go
@@ -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
@@ -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
@@ -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)
@@ -54,25 +57,15 @@ func TestCheckE2E(t *testing.T) {
 		},
 
 		{
-			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},
-			},
-		},
-
-		{
-			description: "check returns all new versions since the last",
-			source: resource.Source{
-				Repository:  "itsdalmo/test-repository",
-				AccessToken: os.Getenv("GITHUB_ACCESS_TOKEN"),
-			},
-			version: resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime},
-			expected: resource.CheckResponse{
+				resource.Version{PR: firstPullRequestID, Commit: firstCommitID, CommittedDate: firstDateTime},
+				resource.Version{PR: targetPullRequestID, Commit: targetCommitID, CommittedDate: targetDateTime},
 				resource.Version{PR: latestPullRequestID, Commit: latestCommitID, CommittedDate: latestDateTime},
 			},
 		},