diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index 7f41f08f635b9..06d7ec6ab5093 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -382,20 +382,41 @@ export class PullRequestLinter { && review.state === 'APPROVED', ); - const communityApproved = reviews.data.some( - review => this.getTrustedCommunityMembers().includes(review.user?.login ?? '') - && review.state === 'APPROVED', - ); + // NOTE: community reviewers may approve, comment, or request changes; however, it + // is possible for the same member to perform any combination of those actions on + // a single PR. We solve this by: + // 1. Filtering reviews to those by trusted community members + // 2. Filtering out reviews that only leave comments (without approving or requesting changes). + // This allows a reviewer to participate in a conversation about their review without + // effectively dismissing their review. While GitHub does not allow community reviewers + // to dismiss their reviews (which requires privileges on the repo), they can leave a + // new review with the opposite approve/request state to update their review. + // 3. Mapping reviewers to only their newest review + // 4. Checking if any reviewers' most recent review is an approval + // -> If so, the PR is considered community approved; the approval can always + // 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 + .filter(review => this.getTrustedCommunityMembers().includes(review.user?.login ?? '')) + .filter(review => review.state !== 'PENDING' && review.state !== 'COMMENTED') + .reduce((grouping, review) => { + // submitted_at is not present for PENDING comments but is present for other states. + // Because of that, it is optional on the type but sure to be present here. Likewise, + // review.user is sure to be defined because we're operating on reviews by trusted + // community members + let newest = grouping[review.user!.login] ?? review; + if (review.submitted_at! > newest.submitted_at!) { + newest = review; + } - // 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', - ); + return { + ...grouping, + [review.user!.login]: newest, + }; + }, {} as Record); + 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 userRequestsExemption = pr.labels.some(label => (label.name === Exemption.REQUEST_EXEMPTION || label.name === Exemption.REQUEST_CLARIFICATION)); diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index 33746d1249954..9952537a8ff35 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -7,8 +7,10 @@ let mockAddLabel = jest.fn(); let mockListReviews = jest.fn().mockImplementation((_props: { _owner: string, _repo: string, _pull_number: number }) => { return { data: [{ id: 1111122222, user: { login: 'aws-cdk-automation' }, state: 'CHANGES_REQUESTED' }] }; }); + beforeAll(() => { jest.spyOn(console, 'log').mockImplementation(); + jest.spyOn(linter.PullRequestLinter.prototype as any, 'getTrustedCommunityMembers').mockImplementation(() => ['trusted1', 'trusted2', 'trusted3']) process.env.REPO_ROOT = path.join(__dirname, '..', '..', '..', '..'); }); @@ -514,8 +516,8 @@ describe('integration tests required on features', () => { test('with label no error', async () => { labels.push({ name: 'pr-linter/cli-integ-tested' }); const prLinter = configureMock(issue, files); - await prLinter.validatePullRequestTarget(SHA); // THEN: no exception + expect(async () => await prLinter.validatePullRequestTarget(SHA)).resolves; }); test('with aws-cdk-automation author', async () => { @@ -653,8 +655,8 @@ describe('integration tests required on features', () => { mockListReviews.mockImplementation(() => { return { data: [ - { id: 1111122222, user: { login: 'aws-cdk-automation' }, state: 'CHANGES_REQUESTED' }, - { id: 1111122223, user: { login: 'someuser' }, author_association: 'MEMBER', state: 'CHANGES_REQUESTED' }, + { id: 1111122222, user: { login: 'aws-cdk-automation' }, state: 'CHANGES_REQUESTED', submitted_at: '2019-11-17T17:43:43Z'}, + { id: 1111122223, user: { login: 'someuser' }, author_association: 'MEMBER', state: 'CHANGES_REQUESTED', submitted_at: '2019-11-18T17:43:43Z' }, ], }; }); @@ -723,7 +725,7 @@ describe('integration tests required on features', () => { mockListReviews.mockImplementation(() => { return { data: [ - { id: 1111122223, user: { login: 'pahud' }, state: 'APPROVED' }, + { id: 1111122223, user: { login: 'trusted1' }, state: 'APPROVED' }, ], }; }); @@ -761,8 +763,9 @@ describe('integration tests required on features', () => { mockListReviews.mockImplementation(() => { return { data: [ - { id: 1111122223, user: { login: 'pahud' }, state: 'COMMENTED' }, - { id: 1111122223, user: { login: 'pahud' }, state: 'APPROVED' }, + { id: 1111122223, user: { login: 'trusted1' }, state: 'APPROVED', submitted_at: '2019-11-18T17:43:43Z' }, + { id: 1111122224, user: { login: 'trusted2' }, state: 'CHANGES_REQUESTED', submitted_at: '2019-11-17T18:43:43Z' }, + { id: 1111122225, user: { login: 'trusted3' }, state: 'COMMENTED', submitted_at: '2019-11-17T19:43:43Z' }, ], }; }); @@ -795,12 +798,12 @@ describe('integration tests required on features', () => { }); }); - test('trusted community member can "request changes" on p2 PR by commenting', async () => { + test('trusted community member can "request changes" on p2 PR by requesting changes', async () => { // GIVEN mockListReviews.mockImplementation(() => { return { data: [ - { id: 1111122223, user: { login: 'pahud' }, state: 'COMMENTED' }, + { id: 1111122223, user: { login: 'trusted1' }, state: 'CHANGES_REQUESTED', submitted_at: '2019-11-17T17:43:43Z' }, ], }; }); @@ -828,6 +831,123 @@ describe('integration tests required on features', () => { expect(mockAddLabel.mock.calls).toEqual([]); }); + test('trusted community member can comment after requesting changes without dismissing', async () => { + // GIVEN + mockListReviews.mockImplementation(() => { + return { + data: [ + { id: 1111122223, user: { login: 'trusted1' }, state: 'CHANGES_REQUESTED', submitted_at: '2019-11-17T17:43:43Z' }, + { id: 1111122224, user: { login: 'trusted1' }, state: 'COMMENTED', submitted_at: '2019-11-18T17:43:43Z' }, + ], + }; + }); + (pr as any).labels = []; + + // 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).toEqual([]); + expect(mockAddLabel.mock.calls).toEqual([]); + }); + + test('trusted community member comments dont mark as "changes requested"', async () => { + // GIVEN + mockListReviews.mockImplementation(() => { + return { + data: [ + { id: 1111122223, user: { login: 'trusted1' }, state: 'COMMENTED', submitted_at: '2019-11-17T17:43:43Z' }, + ], + }; + }); + (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).toEqual([]); + expect(mockAddLabel.mock.calls).toEqual([]); + }); + + test('trusted community members can change own review from approval to requesting changes', async () => { + // GIVEN + mockListReviews.mockImplementation(() => { + return { + data: [ + { id: 1111122223, user: { login: 'trusted1' }, state: 'APPROVED', submitted_at: '2019-11-17T17:43:43Z' }, + { id: 1111122224, user: { login: 'trusted1' }, state: 'CHANGES_REQUESTED', submitted_at: '2019-11-18T17:43:43Z' }, + ] + } + }); + (pr as any).labels = [ + { + name: 'pr/needs-maintainer-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-maintainer-review', + owner: 'aws', + repo: 'aws-cdk', + }); + expect(mockAddLabel.mock.calls).toEqual([]); + }); + + test('trusted community members can change own review from requesting changes to approval', async () => { + // GIVEN + mockListReviews.mockImplementation(() => { + return { + data: [ + { id: 1111122223, user: { login: 'trusted1' }, state: 'CHANGES_REQUESTED', submitted_at: '2019-11-17T17:43:43Z' }, + { id: 1111122224, user: { login: 'trusted1' }, state: 'APPROVED', submitted_at: '2019-11-18T17:43:43Z' }, + ] + } + }); + (pr as any).labels = []; + + // 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).toEqual([]); + expect(mockAddLabel.mock.calls[0][0]).toEqual({ + issue_number: 1234, + labels: ['pr/needs-maintainer-review'], + owner: 'aws', + repo: 'aws-cdk', + }); + }); + test('untrusted community member approval has no affect', async () => { // GIVEN mockListReviews.mockImplementation(() => {