chore(prlinter): fixing some errors in the needs review workflow (#25464

)

Fixing some of the remaining issues with the new needs review workflow

- `aws-cdk-automation` does show up as MEMBER so need to filter it out
- If the PR linter failed then we never made it to the assessReview. Make sure we assess review even if the linter fails

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

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

@@ -310,7 +310,7 @@ export class PullRequestLinter {
    * @param sha the commit sha to evaluate
    */
   private async codeBuildJobSucceeded(sha: string): Promise<boolean> {
-    const statuses = await this.client.rest.repos.listCommitStatusesForRef({
+    const statuses = await this.client.repos.listCommitStatusesForRef({
       owner: this.prParams.owner,
       repo: this.prParams.repo,
       ref: sha,
@@ -340,7 +340,11 @@ export class PullRequestLinter {
     const reviews = await this.client.pulls.listReviews(this.prParams);
     // 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(review => review.author_association === 'MEMBER' && review.state === 'CHANGES_REQUESTED');
+    const maintainerRequestedChanges = reviews.data.some(
+      review => review.author_association === 'MEMBER'
+        && review.user?.login !== 'aws-cdk-automation'
+        && review.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));
     console.log('evaluation: ', JSON.stringify({
@@ -444,16 +448,19 @@ export class PullRequestLinter {
     });
 
     await this.deletePRLinterComment();
-    await this.communicateResult(validationCollector);
-
-    // also assess whether the PR needs review or not
     try {
-      const state = await this.codeBuildJobSucceeded(sha);
-      if (state) {
-        await this.assessNeedsReview(pr);
+      await this.communicateResult(validationCollector);
+      // always assess the review, even if the linter fails
+    } finally {
+      // also assess whether the PR needs review or not
+      try {
+        const state = await this.codeBuildJobSucceeded(sha);
+        if (state) {
+          await this.assessNeedsReview(pr);
+        }
+      } catch (e) {
+        console.log(`assessing review failed for sha ${sha}: `, e);
       }
-    } catch (e) {
-      console.log(`assessing review failed for sha ${sha}: `, e);
     }
   }
 

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

@@ -646,6 +646,40 @@ describe('integration tests required on features', () => {
       });
       expect(mockAddLabel.mock.calls).toEqual([]);
     });
+
+    test('review happens even if linter fails', async () => {
+      // GIVEN
+      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' },
+          ]
+        }
+      });
+      (pr as any).title = 'blah';
+      (pr as any).labels = [
+        {
+          name: 'pr-linter/exemption-requested',
+        },
+        {
+          name: 'pr/needs-review',
+        }
+      ];
+
+      // WHEN
+      const prLinter = configureMock(pr);
+      await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow();
+
+      // THEN
+      expect(mockRemoveLabel.mock.calls[0][0]).toEqual({
+        "issue_number": 1234,
+        "name": "pr/needs-review",
+        "owner": "aws",
+        "repo": "aws-cdk",
+      });
+      expect(mockAddLabel.mock.calls).toEqual([]);
+    });
   });
 });
 
@@ -689,7 +723,7 @@ function configureMock(pr: Subset<linter.GitHubPr>, prFiles?: linter.GitHubFile[
       return {
         data: [{
           context: linter.CODE_BUILD_CONTEXT,
-          state: 'succeeded',
+          state: 'success',
         }],
       }
     },
@@ -708,9 +742,7 @@ function configureMock(pr: Subset<linter.GitHubPr>, prFiles?: linter.GitHubFile[
       pulls: pullsClient as any,
       issues: issuesClient as any,
       search: searchClient as any,
-      rest: {
-        repos: reposClient,
-      },
+      repos: reposClient as any,
       paginate: (method: any, args: any) => { return method(args).data },
     } as any,
   })