chore(prlint): use pr/needs-review label (#25414)

It is currently very difficult to use the issue search functionality to figure out which PRs are ready for review from a core team member. Usually it is ~ 1/3 of open PRs that are in a reviewable state.

Some of the information we need is not available as a search filter in the issue search page (or it is difficult to create a correct filter). A PR is ready for review when:

  1. Not a draft
  2. Does not have any merge conflicts
  3. PR linter is not failing OR the user has requested an exemption
  4. A maintainer has not requested changes ...others may be added in the future

This PR adds functionality to add a label `pr/needs-review` to any PR that meets our criteria. This will allow us to easily use this to filter PRs.

**Note** I tested this locally using `act`, but there is no way to test this for real until it is merged.

----

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

Author: corymhall

.github/workflows/pr-linter.yml

@@ -12,12 +12,15 @@ on:
       - opened
       - synchronize
       - reopened
+  status:
 
 jobs:
   validate-pr:
     permissions:
       contents: read
       pull-requests: write
+      statuses: read
+      issues: read
     runs-on: ubuntu-latest
     steps:
 

CONTRIBUTING.md

@@ -542,6 +542,23 @@ CDK integration tests.
 * Make sure to update the PR title/description if things change. The PR title/description are going to be used as the
   commit title/message and will appear in the CHANGELOG, so maintain them all the way throughout the process.
 
+#### Getting a review from a maintainer
+
+We get A LOT of pull requests, which is a great thing! To help us prioritize
+which pull requests to review we first make sure that the pull request is in a
+mergeable state. This means that the pull request:
+
+1. Is ready for review (not a draft)
+2. Does not have any merge conflicts
+3. Has a passing build
+4. Does not have any requested changes by a maintainer
+5. Has a passing `PR Linter` workflow **OR** the contributor has requested
+   an exemption/clarification.
+
+To make this easier we have a `pr/needs-review` label that we can add to each
+PR. If you do not see this label on your PR then it means that something needs
+to be fixed before it can be reviewed.
+
 #### Adding construct runtime dependencies
 
 Any tool that is not part of the CDK, and needs to be used by a construct during

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

@@ -2,20 +2,35 @@ import * as core from '@actions/core';
 import * as github from '@actions/github';
 import { Octokit } from '@octokit/rest';
 import * as linter from './lint';
+import { StatusEvent } from '@octokit/webhooks-definitions/schema';
 
 async function run() {
   const token: string = process.env.GITHUB_TOKEN!;
   const client = new Octokit({ auth: token });
 
-  const prLinter = new linter.PullRequestLinter({
-    client,
-    owner: github.context.repo.owner,
-    repo: github.context.repo.repo,
-    number: github.context.issue.number,
-  });
-
   try {
-    await prLinter.validate()
+    switch (github.context.eventName) {
+      case 'status':
+        const pr = await linter.PullRequestLinter.getPRFromCommit(client, 'aws', 'aws-cdk', github.context.payload.sha);
+        if (pr) {
+          const prLinter = new linter.PullRequestLinter({
+            client,
+            owner: 'aws',
+            repo: 'aws-cdk',
+            number: pr.number,
+          });
+          await prLinter.validateStatusEvent(pr, github.context.payload as StatusEvent);
+        }
+        break;
+      default:
+        const prLinter = new linter.PullRequestLinter({
+          client,
+          owner: github.context.repo.owner,
+          repo: github.context.repo.repo,
+          number: github.context.issue.number,
+        });
+        await prLinter.validatePullRequestTarget(github.context.payload.sha);
+    }
   } catch (error: any) {
     core.setFailed(error.message);
   }

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

@@ -1,7 +1,15 @@
 import * as path from 'path';
 import { Octokit } from '@octokit/rest';
+import { StatusEvent } from '@octokit/webhooks-definitions/schema';
 import { breakingModules } from './parser';
 import { findModulePath, moduleStability } from './module';
+import { Endpoints } from "@octokit/types";
+
+export type GitHubPr =
+  Endpoints["GET /repos/{owner}/{repo}/pulls/{pull_number}"]["response"]["data"];
+
+
+export const CODE_BUILD_CONTEXT = 'AWS CodeBuild us-east-1 (AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv)';
 
 /**
  * Types of exemption labels in aws-cdk project.
@@ -12,16 +20,14 @@ enum Exemption {
   INTEG_TEST = 'pr-linter/exempt-integ-test',
   BREAKING_CHANGE = 'pr-linter/exempt-breaking-change',
   CLI_INTEG_TESTED = 'pr-linter/cli-integ-tested',
+  REQUEST_CLARIFICATION = 'pr/reviewer-clarification-requested',
+  REQUEST_EXEMPTION = 'pr-linter/exemption-requested',
 }
 
-export interface GitHubPr {
-  readonly number: number;
-  readonly title: string;
-  readonly body: string | null;
-  readonly labels: GitHubLabel[];
-  readonly user?: {
-    login: string;
-  }
+export interface GithubStatusEvent {
+  readonly sha: string;
+  readonly state?: StatusEvent['state'];
+  readonly context?: string;
 }
 
 export interface GitHubLabel {
@@ -172,6 +178,32 @@ export interface PullRequestLinterProps {
  * in the body of the review, and dismiss any previous reviews upon changes to the pull request.
  */
 export class PullRequestLinter {
+  /**
+   * Find an open PR for the given commit.
+   * @param sha the commit sha to find the PR of
+   */
+  public static async getPRFromCommit(client: Octokit, owner: string, repo: string, sha: string): Promise<GitHubPr | undefined> {
+    const prs = await client.search.issuesAndPullRequests({
+      q: sha,
+    });
+    const foundPr = prs.data.items.find(pr => pr.state === 'open');
+    if (foundPr) {
+      // need to do this because the list PR response does not have
+      // all the necessary information
+      const pr = (await client.pulls.get({
+        owner,
+        repo,
+        pull_number: foundPr.number,
+      })).data;
+      const latestCommit = pr.statuses_url.split('/').pop();
+      // only process latest commit
+      if (latestCommit === sha) {
+        return pr;
+      }
+    }
+    return;
+  }
+
   private readonly client: Octokit;
   private readonly prParams: { owner: string, repo: string, pull_number: number };
   private readonly issueParams: { owner: string, repo: string, issue_number: number };
@@ -272,11 +304,91 @@ export class PullRequestLinter {
     }
   }
 
+  /**
+   * Whether or not the codebuild job for the given commit is successful
+   *
+   * @param sha the commit sha to evaluate
+   */
+  private async codeBuildJobSucceeded(sha: string): Promise<boolean> {
+    const statuses = await this.client.rest.repos.listCommitStatusesForRef({
+      owner: this.prParams.owner,
+      repo: this.prParams.repo,
+      ref: sha,
+    });
+    return statuses.data.some(status => status.context === CODE_BUILD_CONTEXT && status.state === 'success');
+  }
+
+  public async validateStatusEvent(pr: GitHubPr, status: StatusEvent): Promise<void> {
+    if (status.context === CODE_BUILD_CONTEXT && status.state === 'success') {
+      await this.assessNeedsReview(pr);
+    }
+  }
+
+  /**
+   * Assess whether or not a PR is ready for review from a core team member.
+   * This is needed because some things that we need to evaluate are not filterable on
+   * the builtin issue search. A PR is ready for review when:
+   *
+   *   1. Not a draft
+   *   2. Does not have any merge conflicts
+   *   3. PR linter is not failing OR the user has requested an exemption
+   *   4. A maintainer has not requested changes
+   */
+  private async assessNeedsReview(
+    pr: Pick<GitHubPr, "mergeable_state" | "draft" | "labels" | "number">,
+  ): Promise<void> {
+    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 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({
+      draft: pr.draft,
+      mergeable_state: pr.mergeable_state,
+      prLinterFailed,
+      maintainerRequestedChanges,
+      userRequestsExemption,
+    }, undefined, 2));
+
+    if (
+      // we don't need to review drafts
+      pr.draft
+        // or PRs with conflicts
+        || pr.mergeable_state === 'dirty'
+        // or PRs that already have changes requested by a maintainer
+        || maintainerRequestedChanges
+        // or the PR linter failed and the user didn't request an exemption
+        || (prLinterFailed && !userRequestsExemption)
+    ) {
+      console.log(`removing labels from pr ${pr.number}`);
+      this.client.issues.removeLabel({
+        owner: this.prParams.owner,
+        repo: this.prParams.repo,
+        issue_number: pr.number,
+        name: 'pr/needs-review',
+      });
+      return;
+    } else {
+      console.log(`adding labels to pr ${pr.number}`);
+      // add needs-review label
+      this.client.issues.addLabels({
+        issue_number: pr.number,
+        owner: this.prParams.owner,
+        repo: this.prParams.repo,
+        labels: [
+          'pr/needs-review',
+        ],
+      });
+      return;
+    }
+  }
+
   /**
    * Performs validations and communicates results via pull request comments, upon failure.
    * This also dismisses previous reviews so they do not remain in REQUEST_CHANGES upon fix of failures.
    */
-  public async validate(): Promise<void> {
+  public async validatePullRequestTarget(sha: string): Promise<void> {
     const number = this.props.number;
 
     console.log(`⌛  Fetching PR number ${number}`);
@@ -331,6 +443,12 @@ export class PullRequestLinter {
 
     await this.deletePRLinterComment();
     await this.communicateResult(validationCollector);
+
+    // also assess whether the PR needs review or not
+    const state = await this.codeBuildJobSucceeded(sha);
+    if (state) {
+      await this.assessNeedsReview(pr);
+    }
   }
 
   private formatErrors(errors: string[]) {

tools/@aws-cdk/prlint/package.json

@@ -13,6 +13,7 @@
   "dependencies": {
     "@actions/core": "^1.10.0",
     "@actions/github": "^5.1.1",
+    "@octokit/webhooks-definitions": "^3.67.3",
     "conventional-commits-parser": "^3.2.4",
     "fs-extra": "^9.1.0",
     "glob": "^7.2.3"