search

shinmyung0 / pullrequest-events-resource

Concourse resource to detect CLOSED and MERGED pull requests on Github
11 stars 12 forks source link

Fetching old PRs #1

Open crstamps2 opened 5 years ago

crstamps2 commented 5 years ago

It seems that this resource starts at the earliest PRs instead of the latest, so it takes some pump priming to get to PRs created after you start using the resource.

shinmyung0 commented 5 years ago

Thanks for the heads up. I think it probably has to do with this ordering: https://github.com/shinmyung0/pullrequest-events-resource/blob/master/scripts/common.js#L160

The order of PRs being fetched is in order of UPDATED_AT in ASC order which...in retrospect might not be the best way. I can put in a fix with some tests when I have time or if you want to file a PR that would also be cool.

crstamps2 commented 5 years ago

I can submit a PR.

shinmyung0 commented 5 years ago

Would the better ordering be PR's that have been updated recently in DESC order?

crstamps2 commented 5 years ago

I am not sure. Maybe it should be configurable?

The use case I am doing is essentially a 1 to 1 ratio. 1 PR merged/closed causes the job to run for that PR, no more, no less.

shinmyung0 commented 5 years ago

yeah I think being configurable is reasonable

jdstone-loansnap commented 5 years ago

@crstamps2 @shinmyung0 has any fix been released for this issue?

jdstone-loansnap commented 5 years ago

It was performing the job on all PRs, not just the first 5 (first: 5), like I had it set to, forward in time. I changed line 160 in common.js to DESC, but now it's just going backwards in time instead of forward and still doing all PRs, not just the first 5.

MadhuvanthG commented 4 years ago

It was performing the job on all PRs, not just the first 5 (first: 5), like I had it set to, forward in time. I changed line 160 in common.js to DESC, but now it's just going backwards in time instead of forward and still doing all PRs, not just the first 5.

+1. We tried using it for a repo that's ~2 years old and it was triggering the pipeline for every single one. If someone can confirm that this is how it would work, I think it's worth a mention in README at least