github-pr-resource
github-pr-resource copied to clipboard
telia-oss
feat: ability to filter on status context existence
Taking a stab at fixing #26, which basically is resulting in commits being missed on check in certain conditions.
The core problem seems to stem from the fact that we are using the commit date to filter out "old" versions. But under certain conditions we may miss some commits on an initial check. Subsequent check will continue to miss the commit as we are now filtering it out based on the date/time of the "latest version".
So instead of using a datetime filter I've added a feature to allow filtering "old commits" based on if they have a status check associated with the SHA. The status check to look for is configured by a new status_check param to the source configuration.
I've been quickly testing this locally and it seems to be working! PR/commit that missed the provided status check are reported on. PR/commit that already have the status check are filtered out.
I left in the commit datetime filtering logic. But if status_check is provided the feature is disabled in favor of using status checks to filter out old commits.
Looking forwards to comments/questions/suggestions!
fixes #26
We've been using this code for about a month now and it's been working great. Would love some feedback when anyone has time to permit. Thanks!
@itsdalmo Do you think we can get this merged? I would like to prevent us from needing to fork the resource to be able to get passed this issue.
@YorickH @jmccann Terribly sorry for the unresponsiveness, if you could resolve the conflicts - I'd love to get this integrated for you guys.
@rickardl I rebased from master and fixed conflicts.
I'll work to update our internal fork and verify it continues to work as expected.
@rickardl So trying it out and initial look is that it's still working. But I'll do some more verifying.
I did run into an issue of checks taking much longer. I think it's related to #233
Hello, is there a chance we could have a new release with this fixed?
Working to update with latest code from master now that #233 is hopefully addressed
Looks like my updates conflict with latest code ... I tried to add them but stuff broke ... need to spend more time on this 😢
Hi @jmccann, which test is failing for you? I rebased onto master with one conflict, and everything is working for me.
@jhosteny The issue I am having is it's not working as expected when I use it in my test environment. The states filter is not being respected so my code seems to be breaking it. It it returning for all states (or maybe just all PRs). Without my code the states is respected and resource runs quickly and returns what I'd expect. So I'm breaking something now. 😢
Are you using it and it seems to be working as expected? 🤔
Think I found the issue ... I didn't rebase and tried to patch in manually and undid some other recent changes ... think I'll have this updated soon
Been using this for several days now and haven't seen any issues. So it's up to a reviewer now on this getting merged. 😄
Been using this for several days now and haven't seen any issues. So it's up to a reviewer now on this getting merged. smile
If it helps, we've also been using this in a private build of the resource for months without issue (along with the submodule fix, #200). We definitely have some repositories whose workflow tends to exercise the issue, and this change fixed it.
Hello, any news if this can be merged? I love the resource - it is very useful but we did get hit by that bug multiple times.
Evening? Is there a chance this PR gets merged? Every week once or twice our team gets hit by the problem of PR's not being picked up.
