Skip to content

Commit b23252b

Browse files
authored
chore(prlint): account for same user with multiple reviews (#26765)
Apparently the same user can have both a `COMMENTED` review and a `APPROVED` review. See #26763 and the logs from its prlinter github action: ``` evaluation: { "draft": false, "mergeable_state": "behind", "maintainerRequestedChanges": false, "maintainerApproved": false, "communityRequestedChanges": true, // also requested changes "communityApproved": true, // approved "userRequestsExemption": false } ``` Also added more logging so that we can see the full data next time. This PR solves the issue by respecting `APPROVED` over `COMMENTED`. Any trusted reviewer who has `APPROVED` a PR will get the PR to `pr/needs-maintainer-review`. Maintainers can always dismiss those reviews if we find that we want to respect someone else's `COMMENTED` review. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent ed9b537 commit b23252b

File tree

2 files changed

+52
-4
lines changed

2 files changed

+52
-4
lines changed

tools/@aws-cdk/prlint/lint.ts

+13-4
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,8 @@ export class PullRequestLinter {
345345
pr: Pick<GitHubPr, 'mergeable_state' | 'draft' | 'labels' | 'number'>,
346346
): Promise<void> {
347347
const reviews = await this.client.pulls.listReviews(this.prParams);
348+
console.log(JSON.stringify(reviews.data));
349+
348350
// NOTE: MEMBER = a member of the organization that owns the repository
349351
// COLLABORATOR = has been invited to collaborate on the repository
350352
const maintainerRequestedChanges = reviews.data.some(
@@ -356,15 +358,22 @@ export class PullRequestLinter {
356358
review => review.author_association === 'MEMBER'
357359
&& review.state === 'APPROVED',
358360
);
359-
const communityRequestedChanges = reviews.data.some(
360-
review => this.getTrustedCommunityMembers().includes(review.user?.login ?? '')
361-
&& review.state === 'COMMENTED', // community members can only approve or comment
362-
);
361+
363362
const communityApproved = reviews.data.some(
364363
review => this.getTrustedCommunityMembers().includes(review.user?.login ?? '')
365364
&& review.state === 'APPROVED',
366365
);
367366

367+
// NOTE: community members can only approve or comment, but it is possible
368+
// for the same member to have both an approved review and a commented review.
369+
// we solve this issue by turning communityRequestedChanges to false if
370+
// communityApproved is true. We can always dismiss an approved review if we want
371+
// to respect someone else's requested changes.
372+
const communityRequestedChanges = communityApproved ? false : reviews.data.some(
373+
review => this.getTrustedCommunityMembers().includes(review.user?.login ?? '')
374+
&& review.state === 'COMMENTED',
375+
);
376+
368377
const prLinterFailed = reviews.data.find((review) => review.user?.login === 'aws-cdk-automation' && review.state !== 'DISMISSED') as Review;
369378
const userRequestsExemption = pr.labels.some(label => (label.name === Exemption.REQUEST_EXEMPTION || label.name === Exemption.REQUEST_CLARIFICATION));
370379
console.log('evaluation: ', JSON.stringify({

tools/@aws-cdk/prlint/test/lint.test.ts

+39
Original file line numberDiff line numberDiff line change
@@ -738,6 +738,45 @@ describe('integration tests required on features', () => {
738738
});
739739
});
740740

741+
test('needs a maintainer review if a community member has approved p2, regardless of other community reviews', async () => {
742+
// GIVEN
743+
mockListReviews.mockImplementation(() => {
744+
return {
745+
data: [
746+
{ id: 1111122223, user: { login: 'pahud' }, state: 'COMMENTED' },
747+
{ id: 1111122223, user: { login: 'pahud' }, state: 'APPROVED' },
748+
],
749+
};
750+
});
751+
(pr as any).labels = [
752+
{
753+
name: 'pr/needs-community-review',
754+
},
755+
];
756+
757+
// WHEN
758+
const prLinter = configureMock(pr);
759+
await prLinter.validateStatusEvent(pr as any, {
760+
sha: SHA,
761+
context: linter.CODE_BUILD_CONTEXT,
762+
state: 'success',
763+
} as any);
764+
765+
// THEN
766+
expect(mockRemoveLabel.mock.calls[0][0]).toEqual({
767+
issue_number: 1234,
768+
name: 'pr/needs-community-review',
769+
owner: 'aws',
770+
repo: 'aws-cdk',
771+
});
772+
expect(mockAddLabel.mock.calls[0][0]).toEqual({
773+
issue_number: 1234,
774+
labels: ['pr/needs-maintainer-review'],
775+
owner: 'aws',
776+
repo: 'aws-cdk',
777+
});
778+
});
779+
741780
test('trusted community member can "request changes" on p2 PR by commenting', async () => {
742781
// GIVEN
743782
mockListReviews.mockImplementation(() => {

0 commit comments

Comments
 (0)