Skip to content

Commit

Permalink
fix(prlint): a review label doesn't appear when a PR is approved if t…
Browse files Browse the repository at this point in the history
…here are too many comments (#31290)

### Issue # (if applicable)

Closes #31294 .

### Reason for this change



I've reviewed and approved [this PR](#30920) as a Trusted Community Reviewer.

But it doesn't get the `pr/needs-maintainer-review` label. It seems to be in `CHANGES_REQUESTED` state and `communityApproved` is also false in the job `PR Linter / validate-pr`. (Please see [this comment in the PR](#30920 (comment)).)

I checked [the prlint's log](https://github.com/aws/aws-cdk/blob/main/tools/@aws-cdk/prlint/lint.ts#L377) in [the GitHub Actions output](https://github.com/aws/aws-cdk/actions/runs/10669155243/job/29570426536), and it appears that there is too much history (such as comments) to get all the latest data.

[List reviews for a pull request](https://docs.github.com/en/rest/pulls/reviews?apiVersion=2022-11-28#list-reviews-for-a-pull-request) in GitHub API can get 30 items per page, however, [prlint is not implemented to handle pagination](https://github.com/aws/aws-cdk/blob/main/tools/%40aws-cdk/prlint/lint.ts#L376).

```ts
  private async assessNeedsReview(
    pr: Pick<GitHubPr, 'mergeable_state' | 'draft' | 'labels' | 'number'>,
  ): Promise<void> {
    const reviews = await this.client.pulls.listReviews(this.prParams);
```

Therefore, when there are **more than 30 comments or change requests**, the review label is no longer displayed.

### Description of changes



Use pagination for listReviews in the octokit library.

https://github.com/octokit/octokit.js?tab=readme-ov-file#pagination

before

```ts
await this.client.pulls.listReviews(this.prParams);
```

after 

```ts
await this.client.paginate(this.client.pulls.listReviews, this.prParams);
```

### Description of how you validated changes



### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
go-to-k authored Sep 3, 2024
1 parent f3bc16c commit 1c63070
Showing 1 changed file with 7 additions and 7 deletions.
14 changes: 7 additions & 7 deletions tools/@aws-cdk/prlint/lint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -373,17 +373,17 @@ export class PullRequestLinter {
private async assessNeedsReview(
pr: Pick<GitHubPr, 'mergeable_state' | 'draft' | 'labels' | 'number'>,
): Promise<void> {
const reviews = await this.client.pulls.listReviews(this.prParams);
console.log(JSON.stringify(reviews.data));
const reviewsData = await this.client.paginate(this.client.pulls.listReviews, this.prParams);
console.log(JSON.stringify(reviewsData));

// NOTE: MEMBER = a member of the organization that owns the repository
// COLLABORATOR = has been invited to collaborate on the repository
const maintainerRequestedChanges = reviews.data.some(
const maintainerRequestedChanges = reviewsData.some(
review => review.author_association === 'MEMBER'
&& review.user?.login !== 'aws-cdk-automation'
&& review.state === 'CHANGES_REQUESTED',
);
const maintainerApproved = reviews.data.some(
const maintainerApproved = reviewsData.some(
review => review.author_association === 'MEMBER'
&& review.state === 'APPROVED',
);
Expand All @@ -403,7 +403,7 @@ export class PullRequestLinter {
// be dismissed by a maintainer to respect another reviewer's requested changes.
// 5. Checking if any reviewers' most recent review requested changes
// -> If so, the PR is considered to still need changes to meet community review.
const reviewsByTrustedCommunityMembers = reviews.data
const reviewsByTrustedCommunityMembers = reviewsData
.filter(review => this.getTrustedCommunityMembers().includes(review.user?.login ?? ''))
.filter(review => review.state !== 'PENDING' && review.state !== 'COMMENTED')
.reduce((grouping, review) => {
Expand All @@ -420,12 +420,12 @@ export class PullRequestLinter {
...grouping,
[review.user!.login]: newest,
};
}, {} as Record<string, typeof reviews.data[0]>);
}, {} as Record<string, typeof reviewsData[0]>);
console.log('raw data: ', JSON.stringify(reviewsByTrustedCommunityMembers));
const communityApproved = Object.values(reviewsByTrustedCommunityMembers).some(({state}) => state === 'APPROVED');
const communityRequestedChanges = !communityApproved && Object.values(reviewsByTrustedCommunityMembers).some(({state}) => state === 'CHANGES_REQUESTED')

const prLinterFailed = reviews.data.find((review) => review.user?.login === 'aws-cdk-automation' && review.state !== 'DISMISSED') as Review;
const prLinterFailed = reviewsData.find((review) => review.user?.login === 'aws-cdk-automation' && review.state !== 'DISMISSED') as Review;
const userRequestsExemption = pr.labels.some(label => (label.name === Exemption.REQUEST_EXEMPTION || label.name === Exemption.REQUEST_CLARIFICATION));
console.log('evaluation: ', JSON.stringify({
draft: pr.draft,
Expand Down

0 comments on commit 1c63070

Please sign in to comment.