Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: update workflow to not close unreviewed stale PRs #3

Closed
wants to merge 2 commits into from
Closed

chore: update workflow to not close unreviewed stale PRs #3

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9200abf
chore: update workflow to not close unreviewed stale PRs
paulhcsun Dec 12, 2023
7654cde
change order of checks and fix formatting
paulhcsun Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion src/stale-prs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,10 @@ export class StalePrFinder {
console.log(' Stale since: ', stale.since.toISOString());
console.log(' Warnings: ', Object.fromEntries(warnings)); // Prints nicer

const memberReviewed = await this.memberReviewed(pull.number);

const warning = warnings.get('STALE PR');
if (!warning || warning < stale.since) {
if (!warning || warning < stale.since || !memberReviewed) {
Comment on lines +157 to +160
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct?

This is saying:

  1. If we don't have a warning yet
  2. OR the last warning we gave is from before the start of the current staleness period
  3. 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Beginning a new staleness period
action = 'warn';
} else if (warnings && this.outOfGracePeriod(warning)) {
Expand Down Expand Up @@ -340,6 +342,30 @@ export class StalePrFinder {

return ret;
}

/**
* Returns true if the PR has been reviewed or commented on by a core member.
*/
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, }));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are looking for reviews, why do we check for comments?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


// 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);

if (!(memberComments.length > 0)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turn this into an early return, instead of a large nested if:

if (memberComments.length > 0) {
  return true;
}

// Else check for another source of truth

In general, try to avoid long if blocks if you can; the code will be easier to follow that way.

const reviews = (await this.client.paginate(this.client.rest.pulls.listReviews, { ...this.repo, 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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's being used in the reviewState() method which is where I copied this line from.


return memberReviews > 0;
}

return true;
}
}

interface ReviewState {
Expand Down
Loading