Skip to content

Commit

Permalink
chore(prlint): account for same user with multiple reviews (#26765)
Browse files Browse the repository at this point in the history
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*
  • Loading branch information
kaizencc authored Aug 15, 2023
1 parent ed9b537 commit b23252b
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 4 deletions.
17 changes: 13 additions & 4 deletions tools/@aws-cdk/prlint/lint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,8 @@ export class PullRequestLinter {
pr: Pick<GitHubPr, 'mergeable_state' | 'draft' | 'labels' | 'number'>,
): Promise<void> {
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(
Expand All @@ -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({
Expand Down
39 changes: 39 additions & 0 deletions tools/@aws-cdk/prlint/test/lint.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down

0 comments on commit b23252b

Please sign in to comment.