Skip to content

Commit

Permalink
Close #26: Filter PRs by updated date instead of commit date
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ctreatma committed Jun 8, 2021
1 parent 9ec47e2 commit c9ee589
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 89 deletions.
7 changes: 1 addition & 6 deletions check.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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
Expand Down
53 changes: 38 additions & 15 deletions check_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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]),
},
Expand All @@ -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]),
},
},
Expand All @@ -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]),
},
},
Expand All @@ -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]),
},
},
Expand All @@ -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]),
},
},
Expand All @@ -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]),
},
Expand All @@ -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]),
Expand Down
Loading

0 comments on commit c9ee589

Please sign in to comment.