RFR: 1737: Draft PR for clean backport mistakenly marked as ready

Erik Joelsson erikj at openjdk.org
Wed Jan 11 23:42:58 UTC 2023


On Wed, 11 Jan 2023 22:51:35 GMT, Zhao Song <zsong at openjdk.org> wrote:

> 1. readyForIntegration for normal pr is determined by 
> (1) whether there is any failed check 
> (2) whether there is any additional errors 
> (3) whether there is any uncompleted additional progress 
> (4) whether there is any integration blocker 
> 
> So if there is a draft normal pr, there will always exist a failed check(ReviewersCheck will always fail because the pr is in draft mode and nobody will approve this pr) 
> Therefore, a draft normal pr will not be marked as 'ready' because there is a failed check not because this pr is in draft mode. 
> 
> 2. readyForIntegration for clean backport is determined by 
> (1) whether this pr is ready for review(TooFewReviewersIssue won't make it false) 
> (2) whether there is any additional errors 
> (3) whether there is any uncompleted additional progress 
> (4) whether there is any integration blocker 
> 
> So if there is a draft clean backport pr, readyForIntegration will be true in most cases. 
> Therefore, the clean backport pr will be marked as 'ready' whether it is in draft mode or not.
> 
> In this patch, whether there is an rfr label will determine whether readyforintegration is true or false.

While this will function correctly, I'm not sure using the contents of newLabels is the right thing to do here. I think it would be better to be explicit and check `pr.isDraft()` directly. It makes it clearer when reading the code what the requirements are. The rest of the requirements for readyForReview are already repeated in the readyForIntegration checks.

It would also be possible to more clearly define a boolean for `readyForReview` (maybe as a return value from `updateReadyForReview`?) and use that in this check, but then also remove the redundant checks in `readyForIntegration`.

-------------

PR: https://git.openjdk.org/skara/pull/1456


More information about the skara-dev mailing list