Skip to content

Commit

Permalink
chore(prlint): update second comment to state exemption request has b…
Browse files Browse the repository at this point in the history 
…een submitted (#29000)

### Issue # (if applicable)

Closes #28803

### Reason for this change

If a contributor requests an exemption or clarification for a PR, they will get a second message (due to the comment event) telling them again that they can request an exemption/clarification. This may confuse contributors, especially beginners.

### Description of changes

Detect if an exempt request comment already exists, next comment would mention an request has been submitted and waiting for a maintainer's review.

### Description of how you validated changes

unit tests

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@GavinZZ @TheRealAmazonKendra
GavinZZ authored and TheRealAmazonKendra committed Feb 9, 2024
1 parent 1ac004e commit 53b909f
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 3 deletions.
6 changes: 5 additions & 1 deletion tools/@aws-cdk/prlint/lint.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ export class PullRequestLinter {
* @param existingReview The review created by a previous run of the linter.
*/
private async createOrUpdatePRLinterReview(failureMessages: string[], existingReview?: Review): Promise<void> {
const body = `The pull request linter fails with the following errors:${this.formatErrors(failureMessages)}`
let body = `The pull request linter fails with the following errors:${this.formatErrors(failureMessages)}`
+ '<b>PRs must pass status checks before we can provide a meaningful review.</b>\n\n'
+ 'If you would like to request an exemption from the status checks or clarification on feedback,'
+ ' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.';
Expand All @@ -265,6 +265,10 @@ export class PullRequestLinter {
});
}

const comments = await this.client.issues.listComments();
if (comments.data.find(comment => comment.body?.includes("Exemption Request"))) {
body += '\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.';
}
await this.client.issues.createComment({
...this.issueParams,
body,
Expand Down
73 changes: 71 additions & 2 deletions tools/@aws-cdk/prlint/test/lint.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1039,6 +1039,71 @@ describe('integration tests required on features', () => {
});
});

describe('with existing Exemption Request comment', () => {
test('valid exemption request comment', async () => {
const issue: Subset<linter.GitHubPr> = {
number: 1,
title: 'fix: some title',
body: '',
labels: [{ name: 'pr-linter/exempt-test' }],
user: {
login: 'author',
},
};
const prLinter = configureMock(issue, undefined, ['Exemption Request']);
await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(
'The pull request linter fails with the following errors:' +
'\n\n\t❌ Fixes must contain a change to an integration test file and the resulting snapshot.' +
'\n\n<b>PRs must pass status checks before we can provide a meaningful review.</b>\n\n' +
'If you would like to request an exemption from the status checks or clarification on feedback,' +
' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.' +
'\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.',
);
});

test('valid exemption request with additional context', async () => {
const issue: Subset<linter.GitHubPr> = {
number: 1,
title: 'fix: some title',
body: '',
labels: [{ name: 'pr-linter/exempt-test' }],
user: {
login: 'author',
},
};
const prLinter = configureMock(issue, undefined, ['Exemption Request: \nThe reason is blah blah blah.']);
await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(
'The pull request linter fails with the following errors:' +
'\n\n\t❌ Fixes must contain a change to an integration test file and the resulting snapshot.' +
'\n\n<b>PRs must pass status checks before we can provide a meaningful review.</b>\n\n' +
'If you would like to request an exemption from the status checks or clarification on feedback,' +
' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.' +
'\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.',
);
});

test('valid exemption request with middle exemption request', async () => {
const issue: Subset<linter.GitHubPr> = {
number: 1,
title: 'fix: some title',
body: '',
labels: [{ name: 'pr-linter/exempt-test' }],
user: {
login: 'author',
},
};
const prLinter = configureMock(issue, undefined, ['Random content - Exemption Request - hello world']);
await expect(prLinter.validatePullRequestTarget(SHA)).rejects.toThrow(
'The pull request linter fails with the following errors:' +
'\n\n\t❌ Fixes must contain a change to an integration test file and the resulting snapshot.' +
'\n\n<b>PRs must pass status checks before we can provide a meaningful review.</b>\n\n' +
'If you would like to request an exemption from the status checks or clarification on feedback,' +
' please leave a comment on this PR containing `Exemption Request` and/or `Clarification Request`.' +
'\n\n✅ A exemption request has been requested. Please wait for a maintainer\'s review.',
);
});
});

describe('metadata file changed', () => {
const files: linter.GitHubFile[] = [{
filename: 'packages/aws-cdk-lib/region-info/build-tools/metadata.ts',
Expand Down Expand Up @@ -1074,7 +1139,7 @@ describe('integration tests required on features', () => {
});
});

function configureMock(pr: Subset<linter.GitHubPr>, prFiles?: linter.GitHubFile[]): linter.PullRequestLinter {
function configureMock(pr: Subset<linter.GitHubPr>, prFiles?: linter.GitHubFile[], existingComments?: string[]): linter.PullRequestLinter {
const pullsClient = {
get(_props: { _owner: string, _repo: string, _pull_number: number, _user: { _login: string} }) {
return { data: pr };
Expand Down Expand Up @@ -1103,7 +1168,11 @@ function configureMock(pr: Subset<linter.GitHubPr>, prFiles?: linter.GitHubFile[
deleteComment() {},

listComments() {
return { data: [{ id: 1212121212, user: { login: 'aws-cdk-automation' }, body: 'The pull request linter fails with the following errors:' }] };
const data = [{ id: 1212121212, user: { login: 'aws-cdk-automation' }, body: 'The pull request linter fails with the following errors:' }];
if (existingComments) {
existingComments.forEach(comment => data.push({ id: 1212121211, user: { login: 'aws-cdk-automation' }, body: comment }));
}
return { data };
},

removeLabel: mockRemoveLabel,
Expand Down

0 comments on commit 53b909f

Please sign in to comment.