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

prlint: a review label doesn't appear when a PR is approved if there are too many comments #31294

Closed
1 task
go-to-k opened this issue Sep 3, 2024 · 2 comments · Fixed by #31290 or softwaremill/tapir#4137 · May be fixed by NOUIY/aws-solutions-constructs#128, NOUIY/aws-solutions-constructs#129 or NOUIY/aws-solutions-constructs#130
Closed
1 task

prlint: a review label doesn't appear when a PR is approved if there are too many comments #31294

go-to-k opened this issue Sep 3, 2024 · 2 comments · Fixed by #31290 or softwaremill/tapir#4137 · May be fixed by NOUIY/aws-solutions-constructs#128, NOUIY/aws-solutions-constructs#129 or NOUIY/aws-solutions-constructs#130
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort p2 package/tools Related to AWS CDK Tools or CLI

Comments

@go-to-k
Copy link
Contributor

go-to-k commented Sep 3, 2024 •
edited
Loading

Describe the bug

I've reviewed and approved this PR 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.)

I checked the prlint's log in the GitHub Actions output, and it appears that there is too much history (such as comments) to get all the latest data.

List reviews for a pull request in GitHub API can get 30 items per page, however, prlint is not implemented to handle pagination.

  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.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

The pr/needs-maintainer-review label appears when a PR is approved even if there are too many comments or change requests.

Current Behavior

The label doesn't appear when a PR is approved if there are too many comments or change requests.

Reproduction Steps

  • Submit a PR
  • Make more than 30 comments or CHANGES_REQUESTED by a trusted community reviewer
  • Approve by the reviewer

Possible Solution

Use pagination for listReviews in the octokit library.

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

before

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

after

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

Additional Information/Context

PRs that have been unlabelled, probably as a result of this issue.

CDK CLI Version

v2.155.0

Framework Version

No response

Node.js Version

18

OS

Depending on GitHub Actions

Language

TypeScript

Language Version

No response

Other information

No response
@go-to-k go-to-k added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 3, 2024
@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Sep 3, 2024
@go-to-k go-to-k mentioned this issue Sep 3, 2024
Merged
1 task
@ashishdhingra ashishdhingra added p2 investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Sep 3, 2024
@ashishdhingra ashishdhingra self-assigned this Sep 3, 2024
@ashishdhingra
Copy link
Contributor

@go-to-k Thanks the reporting the issue and the PR. I have communicated this to the team for review.

@ashishdhingra ashishdhingra added effort/small Small work item – less than a day of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Sep 3, 2024
@ashishdhingra ashishdhingra removed their assignment Sep 3, 2024
@mergify mergify bot closed this as completed in #31290 Sep 3, 2024
mergify bot pushed a commit that referenced this issue Sep 3, 2024
@go-to-k
fix(prlint): a review label doesn't appear when a PR is approved if t…
1c63070 
…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*
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 3, 2024

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2024
pahud pushed a commit to pahud/aws-cdk that referenced this issue Sep 9, 2024
@go-to-k @pahud
fix(prlint): a review label doesn't appear when a PR is approved if t…
83cf880 
…here are too many comments (aws#31290)

### Issue # (if applicable)

Closes aws#31294 .

### Reason for this change



I've reviewed and approved [this PR](aws#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](aws#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*
xazhao pushed a commit to xazhao/aws-cdk that referenced this issue Sep 12, 2024
@go-to-k @xazhao
fix(prlint): a review label doesn't appear when a PR is approved if t…
54ad937 
…here are too many comments (aws#31290)

### Issue # (if applicable)

Closes aws#31294 .

### Reason for this change



I've reviewed and approved [this PR](aws#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](aws#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*
GavinZZ pushed a commit that referenced this issue Sep 12, 2024
@go-to-k @GavinZZ
fix(prlint): a review label doesn't appear when a PR is approved if t…
4eda2f0 
…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*
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort p2 package/tools Related to AWS CDK Tools or CLI
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(prlint): a review label doesn't appear when a PR is approved if there are too many comments go-to-k/aws-cdk
[Snyk] Upgrade aws-cdk-lib from 2.80.0 to 2.162.1 softwaremill/tapir
[Snyk] Upgrade @aws-cdk/cloudformation-diff from 2.99.1 to 2.156.0 NOUIY/aws-solutions-constructs
[Snyk] Upgrade aws-cdk from 2.99.1 to 2.156.0 NOUIY/aws-solutions-constructs
[Snyk] Upgrade @aws-cdk/cloudformation-diff from 2.99.1 to 2.157.0 NOUIY/aws-solutions-constructs
[Snyk] Upgrade aws-cdk from 2.99.1 to 2.157.0 NOUIY/aws-solutions-constructs
[Snyk] Upgrade @aws-cdk/cloudformation-diff from 2.99.1 to 2.158.0 NOUIY/aws-solutions-constructs
[Snyk] Upgrade aws-cdk from 2.99.1 to 2.158.0 NOUIY/aws-solutions-constructs
[Snyk] Upgrade aws-cdk-lib from 2.101.0 to 2.160.0 ajobayer/aws-secure-environment-accelerator
[Snyk] Upgrade @aws-cdk/cloudformation-diff from 2.99.1 to 2.162.1 NOUIY/aws-solutions-constructs
2 participants
@go-to-k @ashishdhingra