chore: update workflow to not close unreviewed stale PRs #3
Conversation
src/stale-prs.ts
Outdated
| const reviews = (await this.client.paginate(this.client.rest.pulls.listReviews, { ...this.repo, pull_number })); | ||
|
|
||
| const comments = await this.client.paginate(this.client.rest.issues.listComments, { | ||
| ...this.repo, | ||
| issue_number: pull_number, | ||
| }); |
There was a problem hiding this comment.
Will all reviews leave comments? If so do we need to look through the reviews?
There was a problem hiding this comment.
Nit: these lines are formatted differently.
There was a problem hiding this comment.
Ya I wasn't 100% sure if issues.listComments would also include reviews hence the separate check, though I don't think it'd hurt to keep the check for reviews.
src/stale-prs.ts
Outdated
| const reviews = (await this.client.paginate(this.client.rest.pulls.listReviews, { ...this.repo, pull_number })); | ||
|
|
||
| const comments = await this.client.paginate(this.client.rest.issues.listComments, { | ||
| ...this.repo, | ||
| issue_number: pull_number, | ||
| }); | ||
|
|
||
| // Reviews by team members | ||
| // Filtering out instances where submitted_at is empty | ||
| const memberReviews = reviews.filter(r => r.author_association === 'MEMBER').filter(r => r.submitted_at); | ||
|
|
||
| // Comments by team members | ||
| // Filtering out instances where submitted_at is empty | ||
| const memberComments = comments.filter(r => r.author_association === 'MEMBER').filter(r => r.submitted_at); | ||
|
|
||
| return memberComments.length > 0 || memberReviews.length > 0; |
There was a problem hiding this comment.
If we are looking for either reviews or comments, can we do them one at a time. Why do we need to request all of the reviews and all of the comments all at once? Say we find a comment from a core team member, then we do not need to make a request for the reviews.
There was a problem hiding this comment.
Good point, I'll change this to only check reviews if after confirming there are no core member comments.
| */ | ||
| private async memberReviewed(pull_number: number): Promise<Boolean> { | ||
|
|
||
| const comments = (await this.client.paginate(this.client.rest.issues.listComments, { ...this.repo, issue_number: pull_number, })); |
There was a problem hiding this comment.
If we are looking for reviews, why do we check for comments?
There was a problem hiding this comment.
I also check for comments in case a core member only decides to just leave a comment on the PR (maybe to answer a question or point the contributor in the right direction) without actually submitting a review. In this case we've still responded to the PR and now it's up to the contributor to continue. I'm not sure how often this would occur though, perhaps just looking at reviews is sufficient?
| // Filtering out instances where submitted_at is empty | ||
| const memberComments = comments.filter(r => r.author_association === 'MEMBER').filter(r => r.submitted_at); | ||
|
|
||
| if (!(memberComments.length > 0)) { |
There was a problem hiding this comment.
Turn this into an early return, instead of a large nested if:
if (memberComments.length > 0) {
return true;
}
// Else check for another source of truthIn general, try to avoid long if blocks if you can; the code will be easier to follow that way.
|
|
||
| // Reviews by team members | ||
| // Filtering out instances where submitted_at is empty | ||
| const memberReviews = reviews.filter(r => r.author_association === 'MEMBER').filter(r => r.submitted_at); |
There was a problem hiding this comment.
I think I can remember that author_association is not visible if the calling account is not part of the target organization; and our bots are not members of the target organization (mandated by our security department).
Are we using this condition elsewhere already?
There was a problem hiding this comment.
Yes, it's being used in the reviewState() method which is where I copied this line from.
| const memberReviewed = await this.memberReviewed(pull.number); | ||
|
|
||
| const warning = warnings.get('STALE PR'); | ||
| if (!warning || warning < stale.since) { | ||
| if (!warning || warning < stale.since || !memberReviewed) { |
There was a problem hiding this comment.
Is this correct?
This is saying:
- If we don't have a warning yet
- OR the last warning we gave is from before the start of the current staleness period
- OR the PR hasn't been reviewed yet
THEN we should add a warning and start the timer for closing.
For fresh PRs, the 3rd condition will be automatically true, because at the start nobody will have looked at it yet.
So this would add a staleness warning immediately on every new PR!
I think what you probably want is to have memberReviewed factor into the value of stale: a PR should not be considered stale if it hasn't been reviewed yet.
Have a look at line 115, and think about how your change would interact with the conditions that are laid out there -- then make the changes over there.
There was a problem hiding this comment.
Oh good catch, I was thinking this would only be run if the PR is already stale but the stale variable includes any build failures and merge conflicts as well. I'll update it with your suggestion.
|
After getting some more context on why this workflow was built and reviewing the stale closed PRs that prompted this change, I'm going to decide to close this as I believe the github action is working as intended. The PRs that were closed automatically closed that prompted this PR (aws/aws-cdk#27693, aws/aws-cdk#27756, aws/aws-cdk#27711) were unreviewed but also in a build failing state. A decision could be made in future construct squads to still try to review those PRs and not close them but for now we will keep this action as is. A potential alternative is to instead add a |
Problem:
Currently this github workflow is closing stale PRs even if they have not yet been reviewed by a core member.
Ideally a PR should never sit long enough to go stale without being reviewed but update will ensure that even in that scenario the PR will not be automatically closed.
Solution:
Check for at least 1 core member comment or review on the PR, it there are none then trigger the
warnaction instead of thecloseaction when the PR becomes stale.