chore: make PR linter overwrite previous reviews and delete old comme…

…nts (#33105) Many changes here: - The PR linter now removes the text of previous reviews, so that they are not distracting. - The PR linter now deletes old comments; before the rewrite, it used to create both comments and reviews. The comments should be deleted. - There were a bunch of missing `await`s, add those. - The missing `await`s slipped in because in the past someone tried to turn off *some* linter rules, and in doing so disabled *all* linter rules. For now, just re-enable the ones that have to do with promises. - Fix a typo in a global constant - Only log the relevant details of reviews, instead of the entire GitHub object ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Jan 23, 2025 · 7b9f6c8 · 7b9f6c8
1 parent 809a7f0
commit 7b9f6c8
Show file tree

Hide file tree

Showing 7 changed files with 253 additions and 37 deletions.
diff --git a/tools/@aws-cdk/prlint/.eslintrc.js b/tools/@aws-cdk/prlint/.eslintrc.js
@@ -3,6 +3,18 @@ baseConfig.parserOptions.project = __dirname + '/tsconfig.json';
 module.exports = {
   ...baseConfig,
   rules: {
+    // The line below should have been here, but wasn't. There are now a shitton
+    // of linter warnings that I don't want to deal with. Just get the most important ones.
+    /* ...baseConfig.rules, */
+
+    // One of the easiest mistakes to make
+    '@typescript-eslint/no-floating-promises': ['error'],
+
+    // Make sure that inside try/catch blocks, promises are 'return await'ed
+    // (must disable the base rule as it can report incorrect errors)
+    'no-return-await': 'off',
+    '@typescript-eslint/return-await': 'error',
+
     'no-console': 'off',
     'jest/valid-expect': 'off',
   },

diff --git a/tools/@aws-cdk/prlint/constants.ts b/tools/@aws-cdk/prlint/constants.ts
@@ -1,5 +1,5 @@
 
-export const DEFEAULT_LINTER_LOGIN = 'aws-cdk-automation';
+export const DEFAULT_LINTER_LOGIN = 'aws-cdk-automation';
 
 export const CODE_BUILD_CONTEXT = 'AWS CodeBuild us-east-1 (AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv)';
 

diff --git a/tools/@aws-cdk/prlint/index.ts b/tools/@aws-cdk/prlint/index.ts
@@ -3,7 +3,7 @@ import { Octokit } from '@octokit/rest';
 import { StatusEvent, PullRequestEvent } from '@octokit/webhooks-definitions/schema';
 import { PullRequestLinter } from './lint';
 import { LinterActions } from './linter-base';
-import { DEFEAULT_LINTER_LOGIN } from './constants';
+import { DEFAULT_LINTER_LOGIN } from './constants';
 
 /**
  * Entry point for PR linter
@@ -38,7 +38,7 @@ async function run() {
     repo,
     number,
     // On purpose || instead of ??, also collapse empty string
-    linterLogin: process.env.LINTER_LOGIN || DEFEAULT_LINTER_LOGIN,
+    linterLogin: process.env.LINTER_LOGIN || DEFAULT_LINTER_LOGIN,
   });
 
   let actions: LinterActions | undefined;

diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts
@@ -1,4 +1,3 @@
-import { execSync } from 'child_process';
 import * as path from 'path';
 import { findModulePath, moduleStability } from './module';
 import { breakingModules } from './parser';
@@ -33,7 +32,7 @@ export class PullRequestLinter extends PullRequestLinterBase {
 
   public async validateStatusEvent(status: StatusEvent): Promise<LinterActions> {
     if (status.context === CODE_BUILD_CONTEXT && status.state === 'success') {
-      return await this.assessNeedsReview();
+      return this.assessNeedsReview();
     }
     return {};
   }
@@ -60,7 +59,12 @@ export class PullRequestLinter extends PullRequestLinterBase {
     const pr = await this.pr();
 
     const reviewsData = await this.client.paginate(this.client.pulls.listReviews, this.prParams);
-    console.log(JSON.stringify(reviewsData));
+    console.log(JSON.stringify(reviewsData.map(r => ({
+      user: r.user?.login,
+      state: r.state,
+      author_association: r.author_association,
+      submitted_at: r.submitted_at,
+    })), undefined, 2));
 
     // NOTE: MEMBER = a member of the organization that owns the repository
     // COLLABORATOR = has been invited to collaborate on the repository
@@ -89,8 +93,10 @@ export class PullRequestLinter extends PullRequestLinterBase {
     //         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 trustedCommunityMembers = await this.getTrustedCommunityMembers();
+
     const reviewsByTrustedCommunityMembers = reviewsData
-      .filter(review => this.getTrustedCommunityMembers().includes(review.user?.login ?? ''))
+      .filter(review => trustedCommunityMembers.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.
@@ -171,10 +177,10 @@ export class PullRequestLinter extends PullRequestLinterBase {
    * Trusted community reviewers is derived from the source of truth at this wiki:
    * https://github.com/aws/aws-cdk/wiki/CDK-Community-PR-Reviews
    */
-  private getTrustedCommunityMembers(): string[] {
+  private async getTrustedCommunityMembers(): Promise<string[]> {
     if (this.trustedCommunity.length > 0) { return this.trustedCommunity; }
 
-    const wiki = execSync('curl https://raw.githubusercontent.com/wiki/aws/aws-cdk/CDK-Community-PR-Reviews.md', { encoding: 'utf-8' }).toString();
+    const wiki = await (await fetch('https://raw.githubusercontent.com/wiki/aws/aws-cdk/CDK-Community-PR-Reviews.md')).text();
     const rawMdTable = wiki.split('<!--section-->')[1].split('\n').filter(l => l !== '');
     for (let i = 2; i < rawMdTable.length; i++) {
       this.trustedCommunity.push(rawMdTable[i].split('|')[1].trim());
@@ -247,6 +253,9 @@ export class PullRequestLinter extends PullRequestLinterBase {
       ],
     });
 
+    // We always delete all comments; in the future we will just communicate via reviews.
+    ret.deleteComments = await this.findExistingPRLinterComments();
+
     ret = mergeLinterActions(ret, await this.validationToActions(validationCollector));
 
     // also assess whether the PR needs review or not
@@ -270,7 +279,7 @@ export class PullRequestLinter extends PullRequestLinterBase {
    */
   private async validationToActions(result: ValidationCollector): Promise<LinterActions> {
     if (result.isValid()) {
-      console.log('✅  Success');
+      console.log('✅ Success');
       return {
         dismissPreviousReview: true,
       };

diff --git a/tools/@aws-cdk/prlint/linter-base.ts b/tools/@aws-cdk/prlint/linter-base.ts
@@ -1,5 +1,5 @@
 import { Octokit } from '@octokit/rest';
-import { GitHubPr, Review } from "./github";
+import { GitHubComment, GitHubPr, Review } from "./github";
 
 export const PR_FROM_MAIN_ERROR = 'Pull requests from `main` branch of a fork cannot be accepted. Please reopen this contribution from another branch on your fork. For more information, see https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-4-pull-request.';
 
@@ -96,25 +96,29 @@ export class PullRequestLinterBase {
     const pr = await this.pr();
 
     for (const label of actions.removeLabels ?? []) {
-      this.removeLabel(label, pr);
+      await this.removeLabel(label, pr);
     }
 
     for (const label of actions.addLabels ?? []) {
-      this.addLabel(label, pr);
+      await this.addLabel(label, pr);
+    }
+
+    for (const comment of actions.deleteComments ?? []) {
+      await this.deleteComment(comment);
     }
 
     if (actions.dismissPreviousReview || actions.requestChanges) {
       if (actions.dismissPreviousReview && actions.requestChanges) {
         throw new Error(`It does not make sense to supply both dismissPreviousReview and requestChanges: ${JSON.stringify(actions)}`);
       }
 
-      const existingReviews = await this.findExistingPRLinterReview();
+      const existingReviews = await this.findExistingPRLinterReviews();
 
       if (actions.dismissPreviousReview) {
-        this.dismissPRLinterReviews(existingReviews, 'passing');
+        await this.dismissPRLinterReviews(existingReviews, 'passing');
       }
       if (actions.requestChanges) {
-        this.createOrUpdatePRLinterReview(actions.requestChanges.failures, actions.requestChanges.exemptionRequest, existingReviews);
+        await this.createOrUpdatePRLinterReview(actions.requestChanges.failures, actions.requestChanges.exemptionRequest, existingReviews);
       }
     }
   }
@@ -150,7 +154,14 @@ export class PullRequestLinterBase {
 
     for (const existingReview of existingReviews ?? []) {
       try {
-        console.log('Dismissing review');
+        console.log(`👉 Dismissing review ${existingReview.id}`);
+        // Make sure to update the old review texts. Dismissing won't do that,
+        // and having multiple messages in place is confusing.
+        await this.client.pulls.updateReview({
+          ...this.prParams,
+          review_id: existingReview.id,
+          body: '(This review is outdated)',
+        });
         await this.client.pulls.dismissReview({
           ...this.prParams,
           review_id: existingReview.id,
@@ -167,11 +178,19 @@ export class PullRequestLinterBase {
    * Finds existing review, if present
    * @returns Existing review, if present
    */
-  private async findExistingPRLinterReview(): Promise<Review[]> {
+  private async findExistingPRLinterReviews(): Promise<Review[]> {
     const reviews = await this.client.paginate(this.client.pulls.listReviews, this.prParams);
     return reviews.filter((review) => review.user?.login === this.linterLogin && review.state !== 'DISMISSED');
   }
 
+  /**
+   * Return all existing comments made by the PR linter
+   */
+  public async findExistingPRLinterComments(): Promise<GitHubComment[]> {
+    const comments = await this.client.paginate(this.client.issues.listComments, this.issueParams);
+    return comments.filter((x) => x.user?.login === this.linterLogin && x.body?.includes('pull request linter'));
+  }
+
   /**
    * Creates a new review and comment for first run with failure or creates a new comment with new failures for existing reviews.
    *
@@ -199,22 +218,22 @@ export class PullRequestLinterBase {
     const body = paras.join('\n\n');
 
     // Dismiss every review except the last one
-    this.dismissPRLinterReviews((existingReviews ?? []).slice(0, -1), 'stale');
+    await this.dismissPRLinterReviews((existingReviews ?? []).slice(0, -1), 'stale');
 
     // Update the last review
     const existingReview = (existingReviews ?? []).slice(-1)[0];
 
     if (!existingReview) {
-      console.log('Creating review');
+      console.log('👉 Creating new request-changes review');
       await this.client.pulls.createReview({
         ...this.prParams,
         event: 'REQUEST_CHANGES',
         body,
       });
     } else if (existingReview.body !== body && existingReview.state === 'CHANGES_REQUESTED') {
       // State is good but body is wrong
-      console.log('Updating review');
-      this.client.pulls.updateReview({
+      console.log(`👉 Updating existing request-changes review: ${existingReview.id}`);
+      await this.client.pulls.updateReview({
         ...this.prParams,
         review_id: existingReview.id,
         body,
@@ -230,8 +249,8 @@ export class PullRequestLinterBase {
       + '(this setting is enabled by default for personal accounts, and cannot be enabled for organization owned accounts). '
       + 'The reason for this is that our automation needs to synchronize your branch with our main after it has been approved, '
       + 'and we cannot do that if we cannot push to your branch.'
-      console.log('Closing pull request');
 
+      console.log('👉 Closing pull request');
       await this.client.issues.createComment({
         ...this.issueParams,
         body: errorMessageBody,
@@ -244,14 +263,27 @@ export class PullRequestLinterBase {
     }
   }
 
+  /**
+   * Delete a comment
+   */
+  private async deleteComment(comment: GitHubComment) {
+    console.log(`👉 Deleting comment ${comment.id}`);
+    await this.client.issues.deleteComment({
+      ...this.issueParams,
+      comment_id: comment.id,
+    });
+  }
+
   private formatErrors(errors: string[]) {
     return errors.map(e => `    ❌ ${e}`).join('\n');
   }
 
-  private addLabel(label: string, pr: Pick<GitHubPr, 'labels' | 'number'>) {
+  private async addLabel(label: string, pr: Pick<GitHubPr, 'labels' | 'number'>) {
     // already has label, so no-op
     if (pr.labels.some(l => l.name === label)) { return; }
-    this.client.issues.addLabels({
+
+    console.log(`👉 Add label ${label}`);
+    await this.client.issues.addLabels({
       issue_number: pr.number,
       owner: this.prParams.owner,
       repo: this.prParams.repo,
@@ -261,10 +293,12 @@ export class PullRequestLinterBase {
     });
   }
 
-  private removeLabel(label: string, pr: Pick<GitHubPr, 'labels' | 'number'>) {
+  private async removeLabel(label: string, pr: Pick<GitHubPr, 'labels' | 'number'>) {
     // does not have label, so no-op
     if (!pr.labels.some(l => l.name === label)) { return; }
-    this.client.issues.removeLabel({
+
+    console.log(`👉 Remove label ${label}`);
+    await this.client.issues.removeLabel({
       issue_number: pr.number,
       owner: this.prParams.owner,
       repo: this.prParams.repo,
@@ -294,6 +328,11 @@ export interface LinterActions {
    */
   removeLabels?: string[];
 
+  /**
+   * Delete the given comments
+   */
+  deleteComments?: GitHubComment[];
+
   /**
    * Post a "request changes" review
    */
@@ -313,6 +352,7 @@ export function mergeLinterActions(a: LinterActions, b: LinterActions): LinterAc
     addLabels: nonEmpty([...(a.addLabels ?? []), ...(b.addLabels ?? [])]),
     removeLabels: nonEmpty([...(a.removeLabels ?? []), ...(b.removeLabels ?? [])]),
     requestChanges: b.requestChanges ?? a.requestChanges,
+    deleteComments: nonEmpty([...(a.deleteComments ?? []), ...(b.deleteComments ?? [])]),
     dismissPreviousReview: b.dismissPreviousReview ?? a.dismissPreviousReview,
   };
 }