diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index 91501eb95e759..7f367705eb692 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -345,6 +345,8 @@ export class PullRequestLinter { pr: Pick, ): Promise { const reviews = await this.client.pulls.listReviews(this.prParams); + console.log(JSON.stringify(reviews.data)); + // 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( @@ -356,15 +358,22 @@ export class PullRequestLinter { review => review.author_association === 'MEMBER' && review.state === 'APPROVED', ); - const communityRequestedChanges = reviews.data.some( - review => this.getTrustedCommunityMembers().includes(review.user?.login ?? '') - && review.state === 'COMMENTED', // community members can only approve or comment - ); + const communityApproved = reviews.data.some( review => this.getTrustedCommunityMembers().includes(review.user?.login ?? '') && review.state === 'APPROVED', ); + // NOTE: community members can only approve or comment, but it is possible + // for the same member to have both an approved review and a commented review. + // we solve this issue by turning communityRequestedChanges to false if + // communityApproved is true. We can always dismiss an approved review if we want + // to respect someone else's requested changes. + const communityRequestedChanges = communityApproved ? false : reviews.data.some( + review => this.getTrustedCommunityMembers().includes(review.user?.login ?? '') + && review.state === 'COMMENTED', + ); + const prLinterFailed = reviews.data.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({ diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index c693af201bc47..50bd81b2c7f2b 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -738,6 +738,45 @@ describe('integration tests required on features', () => { }); }); + test('needs a maintainer review if a community member has approved p2, regardless of other community reviews', async () => { + // GIVEN + mockListReviews.mockImplementation(() => { + return { + data: [ + { id: 1111122223, user: { login: 'pahud' }, state: 'COMMENTED' }, + { id: 1111122223, user: { login: 'pahud' }, state: 'APPROVED' }, + ], + }; + }); + (pr as any).labels = [ + { + name: 'pr/needs-community-review', + }, + ]; + + // WHEN + const prLinter = configureMock(pr); + await prLinter.validateStatusEvent(pr as any, { + sha: SHA, + context: linter.CODE_BUILD_CONTEXT, + state: 'success', + } as any); + + // THEN + expect(mockRemoveLabel.mock.calls[0][0]).toEqual({ + issue_number: 1234, + name: 'pr/needs-community-review', + owner: 'aws', + repo: 'aws-cdk', + }); + expect(mockAddLabel.mock.calls[0][0]).toEqual({ + issue_number: 1234, + labels: ['pr/needs-maintainer-review'], + owner: 'aws', + repo: 'aws-cdk', + }); + }); + test('trusted community member can "request changes" on p2 PR by commenting', async () => { // GIVEN mockListReviews.mockImplementation(() => {