Skip to content

Commit 626e2cc

Browse files
authored
chore(prlint): require consistent scope (without aws-) (#23672)
The AWS CDK uses conventional commits which encourage a scope. In nearly all cases, contributors omit the `aws-` prefix; though there are some situations where they're included (such as #23552, #23225, and others). Interestingly, it seems that most often the `aws-` prefix is used for chore/doc commits that don't end up in changelogs anyway. This actually results in inconsistencies for users in the changelog. Because the changelog sorts entries alphabetically by scope, changes that were contributed with `aws-s3` as the scope are listed at the top of the changelog whereas changes that just used `s3` are sorted further down. This means it's harder for users to review one (or I suppose 2 with Feature/Fix being separate sections) spots in the changelog to identify specific modules they care about. It's also possible that rather than potentially annoying contributors who have to edit their titles, this should be automatically fixed up in the changelog generation. The current behavior may also accidentally encourage users to scan the whole changelog rather than just a few bits of it. So it's very understandable if the potential inconvenience from this change it's worth the minor formatting consistency gain. ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 88fc62d commit 626e2cc

File tree

2 files changed

+83
-0
lines changed

2 files changed

+83
-0
lines changed

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

+26
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,9 @@ export class PullRequestLinter {
306306
validationCollector.validateRuleSet({
307307
testRuleSet: [ { test: validateTitlePrefix } ]
308308
});
309+
validationCollector.validateRuleSet({
310+
testRuleSet: [ { test: validateTitleScope } ]
311+
})
309312

310313
validationCollector.validateRuleSet({
311314
exemption: shouldExemptBreakingChange,
@@ -446,6 +449,29 @@ function hasLabel(pr: GitHubPr, labelName: string): boolean {
446449
return result;
447450
};
448451

452+
/**
453+
* Check that the PR title uses the typical convention for package names.
454+
*
455+
* For example, "fix(s3)" is preferred over "fix(aws-s3)".
456+
*/
457+
function validateTitleScope(pr: GitHubPr): TestResult {
458+
const result = new TestResult();
459+
// Specific commit types are handled by `validateTitlePrefix`. This just checks whether
460+
// the scope includes an `aws-` prefix or not.
461+
// Group 1: Scope with parens - "(aws-<name>)"
462+
// Group 2: Scope name - "aws-<name>"
463+
// Group 3: Preferred scope name - "<name>"
464+
const titleRe = /^\w+(\((aws-([\w_-]+))\))?: /;
465+
const m = titleRe.exec(pr.title);
466+
if (m) {
467+
result.assessFailure(
468+
!!(m[2] && m[3]),
469+
`The title of the pull request should omit 'aws-' from the name of modified packages. Use '${m[3]}' instead of '${m[2]}'.`,
470+
);
471+
}
472+
return result;
473+
}
474+
449475
function assertStability(pr: GitHubPr, _files: GitHubFile[]): TestResult {
450476
const title = pr.title;
451477
const body = pr.body;

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

+57
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,63 @@ describe('breaking changes format', () => {
5858
});
5959
});
6060

61+
describe('commit message format', () => {
62+
test('valid scope', async () => {
63+
const issue = {
64+
number: 1,
65+
title: 'chore(s3): some title',
66+
body: '',
67+
labels: [],
68+
};
69+
const prLinter = configureMock(issue, undefined);
70+
expect(await prLinter.validate()).resolves;
71+
});
72+
73+
test('invalid scope with aws- prefix', async () => {
74+
const issue = {
75+
number: 1,
76+
title: 'fix(aws-s3): some title',
77+
body: '',
78+
labels: [{ name: 'pr-linter/exempt-test' }, { name: 'pr-linter/exempt-integ-test' }],
79+
};
80+
const prLinter = configureMock(issue, undefined);
81+
await expect(prLinter.validate()).rejects.toThrow(/The title of the pull request should omit 'aws-' from the name of modified packages. Use 's3' instead of 'aws-s3'./);
82+
});
83+
84+
test('valid scope with aws- in summary and body', async () => {
85+
const issue = {
86+
number: 1,
87+
title: 'docs(s3): something aws-s3',
88+
body: 'something aws-s3',
89+
labels: [],
90+
};
91+
const prLinter = configureMock(issue, undefined);
92+
expect(await prLinter.validate()).resolves;
93+
});
94+
95+
test('valid with missing scope', async () => {
96+
const issue = {
97+
number: 1,
98+
title: 'docs: something aws-s3',
99+
body: '',
100+
labels: [],
101+
};
102+
const prLinter = configureMock(issue, undefined);
103+
expect(await prLinter.validate()).resolves;
104+
});
105+
106+
test.each(['core', 'prlint', 'awslint'])('valid scope for packages that dont use aws- prefix', async (scope) => {
107+
const issue = {
108+
number: 1,
109+
title: `chore(${scope}): some title`,
110+
body: '',
111+
labels: []
112+
};
113+
const prLinter = configureMock(issue, undefined);
114+
expect(await prLinter.validate()).resolves;
115+
})
116+
});
117+
61118
describe('ban breaking changes in stable modules', () => {
62119
test('breaking change in stable module', async () => {
63120
const issue = {

0 commit comments

Comments
 (0)