From 16ce175d8778e9d4ad74c3d76d8e10df90114ad1 Mon Sep 17 00:00:00 2001 From: Kaizen Conroy Date: Mon, 17 Jul 2023 16:45:57 -0400 Subject: [PATCH 1/2] chore(prlint): allow trusted community reviews --- tools/@aws-cdk/prlint/lint.ts | 129 ++++++++++++++++------- tools/@aws-cdk/prlint/test/lint.test.ts | 133 ++++++++++++++++++++++-- 2 files changed, 215 insertions(+), 47 deletions(-) diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index a326a004f285f..8d0749bd08ab4 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -4,6 +4,7 @@ import { StatusEvent } from '@octokit/webhooks-definitions/schema'; import { breakingModules } from './parser'; import { findModulePath, moduleStability } from './module'; import { Endpoints } from "@octokit/types"; +import { execSync } from 'child_process'; export type GitHubPr = Endpoints["GET /repos/{owner}/{repo}/pulls/{pull_number}"]["response"]["data"]; @@ -205,6 +206,7 @@ export class PullRequestLinter { private readonly client: Octokit; private readonly prParams: { owner: string, repo: string, pull_number: number }; private readonly issueParams: { owner: string, repo: string, issue_number: number }; + private readonly trustedCommunity: string[] = []; constructor(private readonly props: PullRequestLinterProps) { this.client = props.client; @@ -330,6 +332,7 @@ export class PullRequestLinter { * 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 + * 5. A maintainer has not approved */ private async assessNeedsReview( pr: Pick, @@ -346,6 +349,15 @@ 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 !== 'APPROVED' // community members cannot request changes + ) + const communityApproved = reviews.data.some( + review => this.getTrustedCommunityMembers().includes(review.user?.login ?? '') + && review.state === 'APPROVED' + ); + 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({ @@ -356,6 +368,8 @@ export class PullRequestLinter { userRequestsExemption, }, undefined, 2)); + const fixesP1 = pr.labels.some(label => label.name === 'p1'); + let readyForReview = true; if ( // we don't need to review drafts pr.draft @@ -367,32 +381,67 @@ export class PullRequestLinter { || (prLinterFailed && !userRequestsExemption) // or a maintainer has already approved the PR || maintainerApproved + // or a trusted community member has requested changes on the PR + || (!fixesP1 && communityRequestedChanges) ) { - if (pr.labels.some(label => label.name === 'pr/needs-review')) { - 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; + readyForReview = false; + } + + if (readyForReview && (fixesP1 || communityApproved)) { + this.addLabel('pr/needs-maintainer-review', pr); + this.removeLabel('pr/needs-review', pr); + } else if (readyForReview && !fixesP1) { + this.removeLabel('pr/needs-maintainer-review', pr); + this.addLabel('pr/needs-review', pr); } 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; + this.removeLabel('pr/needs-review', pr); + this.removeLabel('pr/needs-maintainer-review', pr); } } + private addLabel(label: string, pr: Pick) { + // already has label + if (pr.labels.some(l => l.name === label)) { return; } + console.log(`adding ${label} 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: [ + label, + ], + }); + } + + private removeLabel(label: string, pr: Pick) { + // does not have label + if (!pr.labels.some(l => l.name === label)) { return; } + console.log(`removing ${label} to pr ${pr.number}`); + // add needs-review label + this.client.issues.removeLabel({ + issue_number: pr.number, + owner: this.prParams.owner, + repo: this.prParams.repo, + name: label, + }); + } + + /** + * Trusted community reviewers is derived from the source of truth at this wiki: + * https://github.com/aws/aws-cdk/wiki/Introducing-CDK-Community-PR-Reviews + */ + private getTrustedCommunityMembers(): string[] { + if (this.trustedCommunity.length > 0) { return this.trustedCommunity; } + + const wiki = execSync('curl https://raw.githubusercontent.com/wiki/aws/aws-cdk/Introducing-CDK-Community-PR-Reviews.md', { encoding: 'utf-8'}).toString(); + const rawMdTable = wiki.split('')[1].split('\n').filter(l => l !== ''); + for (let i = 2; i < rawMdTable.length; i++) { + this.trustedCommunity.push(rawMdTable[i].split('|')[1].trim()); + } + return this.trustedCommunity; + } + /** * 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. @@ -470,7 +519,6 @@ export class PullRequestLinter { private formatErrors(errors: string[]) { return `\n\n\t❌ ${errors.join('\n\t❌ ')}\n\n`; }; - } function isPkgCfnspec(pr: GitHubPr): boolean { @@ -505,49 +553,49 @@ function featureContainsReadme(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && !readmeChanged(files) && !isPkgCfnspec(pr), 'Features must contain a change to a README file.'); return result; -}; +} function featureContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && !testChanged(files), 'Features must contain a change to a test file.'); return result; -}; +} function fixContainsTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFix(pr) && !testChanged(files), 'Fixes must contain a change to a test file.'); return result; -}; +} function featureContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFeature(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files)), 'Features must contain a change to an integration test file and the resulting snapshot.'); return result; -}; +} function fixContainsIntegTest(pr: GitHubPr, files: GitHubFile[]): TestResult { const result = new TestResult(); result.assessFailure(isFix(pr) && (!integTestChanged(files) || !integTestSnapshotChanged(files)), 'Fixes must contain a change to an integration test file and the resulting snapshot.'); return result; -}; +} function shouldExemptReadme(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.README); -}; +} function shouldExemptTest(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.TEST); -}; +} function shouldExemptIntegTest(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.INTEG_TEST); -}; +} function shouldExemptBreakingChange(pr: GitHubPr): boolean { return hasLabel(pr, Exemption.BREAKING_CHANGE); -}; +} function shouldExemptCliIntegTested(pr: GitHubPr): boolean { return (hasLabel(pr, Exemption.CLI_INTEG_TESTED) || pr.user?.login === 'aws-cdk-automation'); @@ -556,8 +604,8 @@ function shouldExemptCliIntegTested(pr: GitHubPr): boolean { function hasLabel(pr: GitHubPr, labelName: string): boolean { return pr.labels.some(function (l: any) { return l.name === labelName; - }) -}; + }); +} /** * Check that the 'BREAKING CHANGE:' note in the body is correct. @@ -566,7 +614,7 @@ function hasLabel(pr: GitHubPr, labelName: string): boolean { * to be said note, but got misspelled as "BREAKING CHANGES:" or * "BREAKING CHANGES(module):" */ - function validateBreakingChangeFormat(pr: GitHubPr, _files: GitHubFile[]): TestResult { +function validateBreakingChangeFormat(pr: GitHubPr, _files: GitHubFile[]): TestResult { const title = pr.title; const body = pr.body; const result = new TestResult(); @@ -579,12 +627,12 @@ function hasLabel(pr: GitHubPr, labelName: string): boolean { result.assessFailure(!titleRe.exec(title), 'The title of this pull request must specify the module name that the first breaking change should be associated to.'); } return result; -}; +} /** * Check that the PR title has the correct prefix. */ - function validateTitlePrefix(pr: GitHubPr): TestResult { +function validateTitlePrefix(pr: GitHubPr): TestResult { const result = new TestResult(); const titleRe = /^(feat|fix|build|chore|ci|docs|style|refactor|perf|test|(r|R)evert)(\([\w_-]+\))?: /; const m = titleRe.exec(pr.title); @@ -592,7 +640,7 @@ function hasLabel(pr: GitHubPr, labelName: string): boolean { !m, "The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/."); return result; -}; +} /** * Check that the PR title uses the typical convention for package names. @@ -625,7 +673,7 @@ function assertStability(pr: GitHubPr, _files: GitHubFile[]): TestResult { const breakingStable = breakingModules(title, body ?? '').filter(mod => 'stable' === moduleStability(findModulePath(mod))); result.assessFailure(breakingStable.length > 0, `Breaking changes in stable modules [${breakingStable.join(', ')}] is disallowed.`); return result; -}; +} function noCliChanges(pr: GitHubPr, files: GitHubFile[]): TestResult { const branch = `pull/${pr.number}/head`; @@ -634,9 +682,10 @@ function noCliChanges(pr: GitHubPr, files: GitHubFile[]): TestResult { return TestResult.fromFailure( cliCodeChanged, - `CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin ${branch} && git push -f origin FETCH_HEAD:test-main-pipeline), then add the '${Exemption.CLI_INTEG_TESTED}' label when the pipeline succeeds.`); + `CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin ${branch} && git push -f origin FETCH_HEAD:test-main-pipeline), then add the '${Exemption.CLI_INTEG_TESTED}' label when the pipeline succeeds.`, + ); } require('make-runnable/custom')({ - printOutputFrame: false + printOutputFrame: false, }); diff --git a/tools/@aws-cdk/prlint/test/lint.test.ts b/tools/@aws-cdk/prlint/test/lint.test.ts index b621d5f9c6eca..f25064404d769 100644 --- a/tools/@aws-cdk/prlint/test/lint.test.ts +++ b/tools/@aws-cdk/prlint/test/lint.test.ts @@ -519,12 +519,12 @@ describe('integration tests required on features', () => { draft: false, mergeable_state: 'behind', number: 1234, - labels: [], + labels: [{ name: 'p2'}], }; beforeEach(() => { mockListReviews.mockImplementation(() => { return { - data: [{ id: 1111122222, user: { login: 'aws-cdk-automation' }, state: 'DISMISSED' }] + data: [{ id: 1111122222, user: { login: 'aws-cdk-automation' }, state: 'DISMISSED' }], } }); }) @@ -548,11 +548,31 @@ describe('integration tests required on features', () => { expect(mockRemoveLabel.mock.calls).toEqual([]); }); + test('needs a review and is p1', async () => { + // WHEN + pr.labels = [{ name: 'p1' }]; + const prLinter = configureMock(pr); + await prLinter.validateStatusEvent(pr as any, { + sha: SHA, + context: linter.CODE_BUILD_CONTEXT, + state: 'success', + } as any); + + // THEN + expect(mockAddLabel.mock.calls[0][0]).toEqual({ + "issue_number": 1234, + "labels": ["pr/needs-maintainer-review"], + "owner": "aws", + "repo": "aws-cdk", + }); + expect(mockRemoveLabel.mock.calls).toEqual([]); + }); + test('does not need a review because of request changes', async () => { // GIVEN mockListReviews.mockImplementation(() => { return { - data: [{ id: 1111122222, user: { login: 'aws-cdk-automation' }, state: 'CHANGES_REQUESTED' }] + data: [{ id: 1111122222, user: { login: 'aws-cdk-automation' }, state: 'CHANGES_REQUESTED' }], } }); (pr as any).labels = [ @@ -583,7 +603,7 @@ describe('integration tests required on features', () => { // GIVEN mockListReviews.mockImplementation(() => { return { - data: [{ id: 1111122222, user: { login: 'aws-cdk-automation' }, state: 'CHANGES_REQUESTED' }] + data: [{ id: 1111122222, user: { login: 'aws-cdk-automation' }, state: 'CHANGES_REQUESTED' }], } }); (pr as any).labels = [ @@ -617,7 +637,7 @@ describe('integration tests required on features', () => { 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).labels = [ @@ -626,7 +646,7 @@ describe('integration tests required on features', () => { }, { name: 'pr/needs-review', - } + }, ]; // WHEN @@ -653,7 +673,40 @@ describe('integration tests required on features', () => { return { data: [ { id: 1111122223, user: { login: 'someuser' }, author_association: 'MEMBER', state: 'APPROVED' }, - ] + ], + } + }); + (pr as any).labels = [ + { + name: 'pr/needs-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-review", + "owner": "aws", + "repo": "aws-cdk", + }); + expect(mockAddLabel.mock.calls).toEqual([]); + }); + + test('needs a maintainer review if a community member has approved p2', async () => { + // GIVEN + mockListReviews.mockImplementation(() => { + return { + data: [ + { id: 1111122223, user: { login: 'pahud' }, state: 'APPROVED' }, + ], } }); (pr as any).labels = [ @@ -677,6 +730,72 @@ describe('integration tests required on features', () => { "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(() => { + return { + data: [ + { id: 1111122223, user: { login: 'pahud' }, state: 'COMMENT' }, + ], + } + }); + (pr as any).labels = [ + { + name: 'pr/needs-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-review", + "owner": "aws", + "repo": "aws-cdk", + }); + expect(mockAddLabel.mock.calls).toEqual([]); + }); + + test('untrusted community member approval has no affect', async () => { + // GIVEN + mockListReviews.mockImplementation(() => { + return { + data: [ + { id: 1111122223, user: { login: 'untrusted' }, state: 'APPROVED' }, + ], + } + }); + (pr as any).labels = [ + { + name: 'pr/needs-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).toEqual([]); expect(mockAddLabel.mock.calls).toEqual([]); }); From c7f7910a781de1195ae1eca4b558fc6f06d42e6f Mon Sep 17 00:00:00 2001 From: Kaizen Conroy Date: Mon, 17 Jul 2023 16:51:29 -0400 Subject: [PATCH 2/2] docs --- tools/@aws-cdk/prlint/lint.ts | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tools/@aws-cdk/prlint/lint.ts b/tools/@aws-cdk/prlint/lint.ts index 8d0749bd08ab4..15b536f059221 100644 --- a/tools/@aws-cdk/prlint/lint.ts +++ b/tools/@aws-cdk/prlint/lint.ts @@ -324,7 +324,7 @@ export class PullRequestLinter { } /** - * Assess whether or not a PR is ready for review from a core team member. + * Assess whether or not a PR is ready for review. * 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: * @@ -333,6 +333,13 @@ export class PullRequestLinter { * 3. PR linter is not failing OR the user has requested an exemption * 4. A maintainer has not requested changes * 5. A maintainer has not approved + * + * In addition, we differentiate between ready for review by a core team member + * (pr/needs-maintainer-review) or ready for review by core OR the trusted community + * (pr/needs-review). A PR is prioritized for core team review when: + * + * 6. It links to a p1 issue + * 7. It links to a p2 issue and has an approved community review */ private async assessNeedsReview( pr: Pick, @@ -381,7 +388,7 @@ export class PullRequestLinter { || (prLinterFailed && !userRequestsExemption) // or a maintainer has already approved the PR || maintainerApproved - // or a trusted community member has requested changes on the PR + // or a trusted community member has requested changes on a p2 PR || (!fixesP1 && communityRequestedChanges) ) { readyForReview = false; @@ -400,10 +407,9 @@ export class PullRequestLinter { } private addLabel(label: string, pr: Pick) { - // already has label + // already has label, so no-op if (pr.labels.some(l => l.name === label)) { return; } console.log(`adding ${label} to pr ${pr.number}`); - // add needs-review label this.client.issues.addLabels({ issue_number: pr.number, owner: this.prParams.owner, @@ -415,10 +421,9 @@ export class PullRequestLinter { } private removeLabel(label: string, pr: Pick) { - // does not have label + // does not have label, so no-op if (!pr.labels.some(l => l.name === label)) { return; } console.log(`removing ${label} to pr ${pr.number}`); - // add needs-review label this.client.issues.removeLabel({ issue_number: pr.number, owner: this.prParams.owner,