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.

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

chore(prlint): don't mark PRs as 'not ready' on community comment #27819

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

laurelmay
Copy link
Contributor

Community reviewers have the ability to choose any of the
approve/comment/request changes buttons that are available in the review
tab. Prior to this change, prlint would consider a comment from a
community reviewer as being equivalent to requesting changes (and in
fact, didn't consider the requesting changes case). This would remove
the pr/needs-community-review label which surprised some reviewers.

With this change, prlint will only remove the
pr/needs-community-review label when a community reviewer specifically
chooses "request changes". Additionally, reviewers are now able to
switch from approving to requesting changes (this doesn't override any
other reviewers' approvals, just that reviewer's own).

Additionally, this adds mocks for the getTrustedCommunityMembers
method and avoids hardcoding the logins of multiple community reviewers
into the tests. As a side effect, the tests also run faster since curl
isn't being invoked so frequently.

Here is a screenshot of what I see when reviewing on this repo:
image


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 bot added p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK labels Nov 3, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team November 3, 2023 05:00
@laurelmay
Copy link
Contributor Author

It also seems unnecessary to filter out reviews from the PR author as GitHub already doesn't allow self-approvals or self-requests (though you can comment). Here is what I see on this PR's review tab:

image

@laurelmay laurelmay force-pushed the prlint-labels-community-review branch from 3272f1d to 8047429 Compare November 3, 2023 05:10
Community reviewers have the ability to choose any of the
approve/comment/request changes buttons that are available in the review
tab. Prior to this change, `prlint` would consider a comment from a
community reviewer as being equivalent to requesting changes (and in
fact, didn't consider the requesting changes case). This would remove
the `pr/needs-community-review` label which surprised some reviewers.

With this change, `prlint` will only remove the
`pr/needs-community-review` label when a community reviewer specifically
chooses "request changes". Additionally, reviewers are now able to
switch from approving to requesting changes (this doesn't override any
other reviewers' approvals, just that reviewer's own).

Additionally, this adds mocks for the `getTrustedCommunityMembers`
method and avoids hardcoding the logins of multiple community reviewers
into the tests. As a side effect, the tests also run faster since `curl`
isn't being invoked so frequently.
@laurelmay laurelmay force-pushed the prlint-labels-community-review branch from 8047429 to 1e24d2e Compare November 3, 2023 05:11
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 3, 2023
@laurelmay
Copy link
Contributor Author

For an example of a community member requesting changes, see #27744 (review). Even though GitHub shows it as "suggested changes", the API state field is still CHANGES_REQUESTED. https://api.github.com/repos/aws/aws-cdk/pulls/27744/reviews

Copy link
Contributor

@go-to-k go-to-k left a comment

Choose a reason for hiding this comment

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

Great! Thank you for addressing this matter.

@aws-cdk-automation aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Nov 7, 2023
@vinayak-kukreja vinayak-kukreja self-assigned this Nov 8, 2023
Copy link
Contributor

@vinayak-kukreja vinayak-kukreja left a comment

Choose a reason for hiding this comment

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

Hey, this is great. Thank you for contributing this 🙌

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: e67e5f9
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

mergify bot commented Nov 8, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 412fb9f into aws:main Nov 8, 2023
10 checks passed
mikewrighton pushed a commit that referenced this pull request Nov 13, 2023
…7819)

Community reviewers have the ability to choose any of the
approve/comment/request changes buttons that are available in the review
tab. Prior to this change, `prlint` would consider a comment from a
community reviewer as being equivalent to requesting changes (and in
fact, didn't consider the requesting changes case). This would remove
the `pr/needs-community-review` label which surprised some reviewers.

With this change, `prlint` will only remove the
`pr/needs-community-review` label when a community reviewer specifically
chooses "request changes". Additionally, reviewers are now able to
switch from approving to requesting changes (this doesn't override any
other reviewers' approvals, just that reviewer's own).

Additionally, this adds mocks for the `getTrustedCommunityMembers`
method and avoids hardcoding the logins of multiple community reviewers
into the tests. As a side effect, the tests also run faster since `curl`
isn't being invoked so frequently.

Here is a screenshot of what I see when reviewing on this repo:
![image](https://github.com/aws/aws-cdk/assets/850893/03d3b5e5-2798-47bf-951d-b72964f974aa)

----

*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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants