Skip to content

Commit

Permalink
chore(prlint): require consistent scope (without aws-)
Browse files Browse the repository at this point in the history
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 aws#23552, aws#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.
  • Loading branch information
kylelaker committed Jan 13, 2023
1 parent dce662c commit 3fe68c4
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 0 deletions.
26 changes: 26 additions & 0 deletions tools/@aws-cdk/prlint/lint.ts
Expand Up @@ -306,6 +306,9 @@ export class PullRequestLinter {
validationCollector.validateRuleSet({
testRuleSet: [ { test: validateTitlePrefix } ]
});
validationCollector.validateRuleSet({
testRuleSet: [ { test: validateTitleScope } ]
})

validationCollector.validateRuleSet({
exemption: shouldExemptBreakingChange,
Expand Down Expand Up @@ -446,6 +449,29 @@ function hasLabel(pr: GitHubPr, labelName: string): boolean {
return result;
};

/**
* Check that the PR title uses the typical convention for package names.
*
* For example, "fix(s3)" is preferred over "fix(aws-s3)".
*/
function validateTitleScope(pr: GitHubPr): TestResult {
const result = new TestResult();
// Specific commit types are handled by `validateTitlePrefix`. This just checks whether
// the scope includes an `aws-` prefix or not.
// Group 1: Scope with parens - "(aws-<name>)"
// Group 2: Scope name - "aws-<name>"
// Group 3: Preferred scope name - "<name>"
const titleRe = /^\w+(\((aws-([\w_-]+))\))?: /;
const m = titleRe.exec(pr.title);
if (m) {
result.assessFailure(
!!(m[2] && m[3]),
`The title of the pull request should omit 'aws-' from the name of modified packages. Use '${m[3]}' instead of '${m[2]}'.`,
);
}
return result;
}

function assertStability(pr: GitHubPr, _files: GitHubFile[]): TestResult {
const title = pr.title;
const body = pr.body;
Expand Down
57 changes: 57 additions & 0 deletions tools/@aws-cdk/prlint/test/lint.test.ts
Expand Up @@ -58,6 +58,63 @@ describe('breaking changes format', () => {
});
});

describe('commit message format', () => {
test('valid scope', async () => {
const issue = {
number: 1,
title: 'chore(s3): some title',
body: '',
labels: [],
};
const prLinter = configureMock(issue, undefined);
expect(await prLinter.validate()).resolves;
});

test('invalid scope with aws- prefix', async () => {
const issue = {
number: 1,
title: 'fix(aws-s3): some title',
body: '',
labels: [{ name: 'pr-linter/exempt-test' }, { name: 'pr-linter/exempt-integ-test' }],
};
const prLinter = configureMock(issue, undefined);
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'./);
});

test('valid scope with aws- in summary and body', async () => {
const issue = {
number: 1,
title: 'docs(s3): something aws-s3',
body: 'something aws-s3',
labels: [],
};
const prLinter = configureMock(issue, undefined);
expect(await prLinter.validate()).resolves;
});

test('valid with missing scope', async () => {
const issue = {
number: 1,
title: 'docs: something aws-s3',
body: '',
labels: [],
};
const prLinter = configureMock(issue, undefined);
expect(await prLinter.validate()).resolves;
});

test.each(['core', 'prlint', 'awslint'])('valid scope for packages that dont use aws- prefix', async (scope) => {
const issue = {
number: 1,
title: `chore(${scope}): some title`,
body: '',
labels: []
};
const prLinter = configureMock(issue, undefined);
expect(await prLinter.validate()).resolves;
})
});

describe('ban breaking changes in stable modules', () => {
test('breaking change in stable module', async () => {
const issue = {
Expand Down

0 comments on commit 3fe68c4

Please sign in to comment.