search

telia-oss / github-pr-resource

Github pull request resource for Concourse
MIT License
182 stars 5 forks source link

Check operation missing eligible PRs due to already-seen threshold moving past them. #204

Open andreas-kupries opened 4 years ago

andreas-kupries commented 4 years ago

Hello. We believe to have found a serious issue in the implementation of this resource which causes it to semi-deterministically ignore eligible PRs. While this issue may look at first glance to be (# 26) it is not the same.

The problem looks to be in the interaction of the internal optimization to quickly skip already seen commits and filtering strategies like required_review_approvals, labels, etc.

The issue is that these filter strategies can make any arbitrarily old PR visible much later, namely when such a PR changes its state, i.e. getting the necessary approvals, getting one of the looked-for labels, etc.

There is a high chance that by the time the PR changes state and thus becomes eligible the already-seen threshold has moved beyond it and thus prevents the remainder of the Check code to see this PR. Despite it being eligible now.

While there are workarounds (See (x) below), these are very inconvenient, and then there is the necessity and problem of actually having to recognize when this issue has happened, and/or having to be continuously on the lookout for such.

IMHO the better solution is to completely remove the already-seen optimization, so that any old PR will be caught when it changes eligibility. This could be mixed with checking if there are filters like required_review_approvals in play at all and disabling the optimization only if that is true.

I can see that this may not be something you wish to do, as it may slow the check down if the set of possible PRs is ever-growing, or simply always quite large. (In our repos the number of open PRs is quite limited all the time, so we have no issue with removing the optimization).

A suitable compromise may be the addition of (another) parameter which enables the user of the resource to control the already-seen optimization at their discretion. This could be a simple boolean, i.e. on/off, or something like a time interval. The latter would control how far back from the actual threshold to still check old PRs for state changes. Then, if it is, for example, expected that a PR will get an approval within 2 days, the parameter can be set accordingly, ensuring that all PRs within 2 days of the threshold are still checked. A special value for the interval could be used to completely disable the threshold, if the user has the desire to do so.

(x) Any git operation which updates the tip of the PR to a commit whose timestamp is before the threshold. I.e. pushing an empty change, or amend the head commit of the PR, etc. And even this is subject to race as it may require re-approval, re-labeling, etc. and while that is done by the user the threshold may have jumped over it again because the resource checked just while that was going on.

ngautha1-ford commented 3 years ago

Hello, we are also facing this issue. Any workarounds so far?

andreas-kupries commented 3 years ago

Oh, a blast from the past ...

Well. We moved from Concourse as our CI to Github Actions. Nothing more needs to be said, I guess.

Sorry that I could not help further.