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

[ctreatma]

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 so that this resource returns all open PRs that match all explicitly-configured filters. Concourse can deduplicate versions based on metadata, which means that it 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.

[rickardl]

@ctreatma I agree on the premise, to delegate this to Concourse, but is there any performance impact doing this, would you be able to provide some form of baseline of how this change might affect people? Also, we changed the behavior recently, in the current master, we instead look at if the PR itself has changed, instead of the most recent Commit. Does that impact the decision you made to remove the check?

If you could provide a resource with your implementation, that I can use, I might be able to get those metrics myself.

[ctreatma]

I haven't run performance tests on this change yet. We ran a couple pipelines with this implementation to confirm that it worked as expected, but given that the resource at the time was limited to discovering open PRs and that we tend to have 10 or fewer PRs open on our repos at any given time, we weren't too concerned about the performance impact.

With the new change to use PR updated date and allow tracking merged & closed PRs, it's possible a resource configuration could return a large list of versions for Concourse to reconcile. I'll work on rebasing my change and will see if I can get it published in a place you can access.

[rickardl]

@ctreatma I would appreciate that, if anyone else looking into this as something they'd like to test out, please feel free to pitch in.

Optionally, we could release this change as a feature flag, since it's small code change, but with a larger impact.

[rickardl]

@ctreatma I spoke to @itsdalmo and we'd love to see this included in v0.22.0, so I'll keep a spot open for it.

[ctreatma]

@rickardl I pushed a build with this feature to Docker Hub as ctreatma/github-pr-resource:no-date-filter.

I'm a bit out of my wheelhouse with performance testing a Concourse resource but happy to dig in. Could you provide some guidance on what metrics would be useful for measuring the impact of this change and how I can collect them from a Concourse instance? I'm figuring I'll run an instance locally for testing, but not sure if that's feasible or useful in this case.

[isaacsanders]

I think this might help me. I would be a fan of merging this.

[Benjamintf1]

I like this pr too!

[ctreatma]

@rickardl I haven't had a chance to figure out local performance testing of this change, but we've been using it for a couple months now with no issues thus far. If I put this change behind a flag so that users have to opt in to it, would that enable us to get it merged in for others to use?

[Pasupathi-Rajamanickam]

I like this PR, many of my PRs are not getting triggered, looking at the code, this could be the only condition that can stop my PRs not getting triggered. Please move this forward or, if I can make this changes somewhere and use it in concourse would love to do it.