Skip to content

Commit c9ee589

Browse files
ctreatmactreatma
committed
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.
1 parent 9ec47e2 commit c9ee589

5 files changed

Lines changed: 127 additions & 89 deletions

File tree

‎check.go‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ func Check(request CheckRequest, manager Github) (CheckResponse, error) {
2020
filterStates = request.Source.States
2121
}
2222

23-
pulls, err := manager.ListPullRequests(filterStates)
23+
pulls, err := manager.ListPullRequests(filterStates, request.Version.CommittedDate)
2424
if err != nil {
2525
return nil, fmt.Errorf("failed to get last commits: %s", err)
2626
}
@@ -44,11 +44,6 @@ Loop:
4444
continue
4545
}
4646

47-
// Filter out commits that are too old.
48-
if !p.UpdatedDate().Time.After(request.Version.CommittedDate) {
49-
continue
50-
}
51-
5247
// Filter out pull request if it does not contain at least one of the desired labels
5348
if len(request.Source.Labels) > 0 {
5449
labelFound := false

‎check_test.go‎

Lines changed: 38 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func TestCheck(t *testing.T) {
5050
},
5151

5252
{
53-
description: "check returns the previous version when its still latest",
53+
description: "check returns all open PRs if there is a previous",
5454
source: resource.Source{
5555
Repository: "itsdalmo/test-repository",
5656
AccessToken: "oauthtoken",
@@ -59,20 +59,13 @@ func TestCheck(t *testing.T) {
5959
pullRequests: testPullRequests,
6060
files: [][]string{},
6161
expected: resource.CheckResponse{
62-
resource.NewVersion(testPullRequests[1]),
63-
},
64-
},
65-
66-
{
67-
description: "check returns all new versions since the last",
68-
source: resource.Source{
69-
Repository: "itsdalmo/test-repository",
70-
AccessToken: "oauthtoken",
71-
},
72-
version: resource.NewVersion(testPullRequests[3]),
73-
pullRequests: testPullRequests,
74-
files: [][]string{},
75-
expected: resource.CheckResponse{
62+
resource.NewVersion(testPullRequests[11]),
63+
resource.NewVersion(testPullRequests[8]),
64+
resource.NewVersion(testPullRequests[7]),
65+
resource.NewVersion(testPullRequests[6]),
66+
resource.NewVersion(testPullRequests[5]),
67+
resource.NewVersion(testPullRequests[4]),
68+
resource.NewVersion(testPullRequests[3]),
7669
resource.NewVersion(testPullRequests[2]),
7770
resource.NewVersion(testPullRequests[1]),
7871
},
@@ -93,6 +86,7 @@ func TestCheck(t *testing.T) {
9386
{"terraform/modules/variables.tf", "travis.yml"},
9487
},
9588
expected: resource.CheckResponse{
89+
resource.NewVersion(testPullRequests[3]),
9690
resource.NewVersion(testPullRequests[2]),
9791
},
9892
},
@@ -112,6 +106,7 @@ func TestCheck(t *testing.T) {
112106
{"terraform/modules/variables.tf", "travis.yml"},
113107
},
114108
expected: resource.CheckResponse{
109+
resource.NewVersion(testPullRequests[3]),
115110
resource.NewVersion(testPullRequests[2]),
116111
},
117112
},
@@ -126,6 +121,15 @@ func TestCheck(t *testing.T) {
126121
version: resource.NewVersion(testPullRequests[1]),
127122
pullRequests: testPullRequests,
128123
expected: resource.CheckResponse{
124+
resource.NewVersion(testPullRequests[11]),
125+
resource.NewVersion(testPullRequests[8]),
126+
resource.NewVersion(testPullRequests[7]),
127+
resource.NewVersion(testPullRequests[6]),
128+
resource.NewVersion(testPullRequests[5]),
129+
resource.NewVersion(testPullRequests[4]),
130+
resource.NewVersion(testPullRequests[3]),
131+
resource.NewVersion(testPullRequests[2]),
132+
resource.NewVersion(testPullRequests[1]),
129133
resource.NewVersion(testPullRequests[0]),
130134
},
131135
},
@@ -140,6 +144,13 @@ func TestCheck(t *testing.T) {
140144
version: resource.NewVersion(testPullRequests[3]),
141145
pullRequests: testPullRequests,
142146
expected: resource.CheckResponse{
147+
resource.NewVersion(testPullRequests[11]),
148+
resource.NewVersion(testPullRequests[8]),
149+
resource.NewVersion(testPullRequests[7]),
150+
resource.NewVersion(testPullRequests[6]),
151+
resource.NewVersion(testPullRequests[5]),
152+
resource.NewVersion(testPullRequests[4]),
153+
resource.NewVersion(testPullRequests[3]),
143154
resource.NewVersion(testPullRequests[1]),
144155
},
145156
},
@@ -154,6 +165,13 @@ func TestCheck(t *testing.T) {
154165
version: resource.NewVersion(testPullRequests[3]),
155166
pullRequests: testPullRequests,
156167
expected: resource.CheckResponse{
168+
resource.NewVersion(testPullRequests[11]),
169+
resource.NewVersion(testPullRequests[8]),
170+
resource.NewVersion(testPullRequests[7]),
171+
resource.NewVersion(testPullRequests[6]),
172+
resource.NewVersion(testPullRequests[5]),
173+
resource.NewVersion(testPullRequests[4]),
174+
resource.NewVersion(testPullRequests[3]),
157175
resource.NewVersion(testPullRequests[2]),
158176
resource.NewVersion(testPullRequests[1]),
159177
},
@@ -169,6 +187,11 @@ func TestCheck(t *testing.T) {
169187
version: resource.NewVersion(testPullRequests[5]),
170188
pullRequests: testPullRequests,
171189
expected: resource.CheckResponse{
190+
resource.NewVersion(testPullRequests[11]),
191+
resource.NewVersion(testPullRequests[8]),
192+
resource.NewVersion(testPullRequests[7]),
193+
resource.NewVersion(testPullRequests[6]),
194+
resource.NewVersion(testPullRequests[5]),
172195
resource.NewVersion(testPullRequests[3]),
173196
resource.NewVersion(testPullRequests[2]),
174197
resource.NewVersion(testPullRequests[1]),

0 commit comments

Comments
 (0)