Skip to content

Commit bef91e1

Browse files
authored
chore(prlint): remove needs-review label on approved prs (#25506)
I figure that approved PRs should also lose the `needs-review` label, since they will still exist in the open PR list for a while and pollute the pool of actual prs needing review. I could've bundled this in to `maintainerRequestsChanges` but I figured it makes more sense to be explicit about the different rules we have for the `needs-review` label. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 1c84790 commit bef91e1

File tree

2 files changed

+39
-2
lines changed

2 files changed

+39
-2
lines changed

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

+6-2
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,6 @@ interface Test {
9999
* Represents a set of tests and the conditions under which those rules exempt.
100100
*/
101101
interface ValidateRuleSetOptions {
102-
103102
/**
104103
* The function to test for exemption from the rules in testRuleSet.
105104
*/
@@ -151,7 +150,6 @@ class ValidationCollector {
151150
* Props used to perform linting against the pull request.
152151
*/
153152
export interface PullRequestLinterProps {
154-
155153
/**
156154
* GitHub client scoped to pull requests. Imported via @actions/github.
157155
*/
@@ -345,6 +343,10 @@ export class PullRequestLinter {
345343
&& review.user?.login !== 'aws-cdk-automation'
346344
&& review.state === 'CHANGES_REQUESTED'
347345
);
346+
const maintainerApproved = reviews.data.some(
347+
review => review.author_association === 'MEMBER'
348+
&& review.state === 'APPROVED'
349+
);
348350
const prLinterFailed = reviews.data.find((review) => review.user?.login === 'aws-cdk-automation' && review.state !== 'DISMISSED') as Review;
349351
const userRequestsExemption = pr.labels.some(label => (label.name === Exemption.REQUEST_EXEMPTION || label.name === Exemption.REQUEST_CLARIFICATION));
350352
console.log('evaluation: ', JSON.stringify({
@@ -364,6 +366,8 @@ export class PullRequestLinter {
364366
|| maintainerRequestedChanges
365367
// or the PR linter failed and the user didn't request an exemption
366368
|| (prLinterFailed && !userRequestsExemption)
369+
// or a maintainer has already approved the PR
370+
|| maintainerApproved
367371
) {
368372
if (pr.labels.some(label => label.name === 'pr/needs-review')) {
369373
console.log(`removing labels from pr ${pr.number}`);

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

+33
Original file line numberDiff line numberDiff line change
@@ -647,6 +647,39 @@ describe('integration tests required on features', () => {
647647
expect(mockAddLabel.mock.calls).toEqual([]);
648648
});
649649

650+
test('does not need a review if member has approved', async () => {
651+
// GIVEN
652+
mockListReviews.mockImplementation(() => {
653+
return {
654+
data: [
655+
{ id: 1111122223, user: { login: 'someuser' }, author_association: 'MEMBER', state: 'APPROVED' },
656+
]
657+
}
658+
});
659+
(pr as any).labels = [
660+
{
661+
name: 'pr/needs-review',
662+
}
663+
];
664+
665+
// WHEN
666+
const prLinter = configureMock(pr);
667+
await prLinter.validateStatusEvent(pr as any, {
668+
sha: SHA,
669+
context: linter.CODE_BUILD_CONTEXT,
670+
state: 'success',
671+
} as any);
672+
673+
// THEN
674+
expect(mockRemoveLabel.mock.calls[0][0]).toEqual({
675+
"issue_number": 1234,
676+
"name": "pr/needs-review",
677+
"owner": "aws",
678+
"repo": "aws-cdk",
679+
});
680+
expect(mockAddLabel.mock.calls).toEqual([]);
681+
});
682+
650683
test('review happens even if linter fails', async () => {
651684
// GIVEN
652685
mockListReviews.mockImplementation(() => {

0 commit comments

Comments
 (0)