Skip to content

Conversation

@dougch
Copy link
Contributor

@dougch dougch commented Oct 31, 2025

Release Summary:

Resolved issues:

resolves none

Description of changes:

Adds a GitHub Action to check the last commit for compliance with conventional commits.

Call-outs:

Shooting for parity with the configuration in https://github.com/aws/s2n-quic.

Testing:

In CI this run should pass...as this PR commit is using conventional commits.

Locally, we can go back in time and find a commit that didn't follow the format:

cd .github
npm install --save-dev @commitlint/config-conventional @commitlint/cli
#Check the last commit
npx commitlint -g ./config/commitlint.config.js --last
npx commitlint -g ./config/commitlint.config.js --from a214bcb9d5e73bec9b760c714c4a57705c9a754f --to e8f51528dece033d7627cf557a9b31fa290d5f77
⧗   input: refactor 2/2: Fix security policy version in tests to numbered string (#5553)
✖   subject may not be empty [subject-empty]
✖   type may not be empty [type-empty]

✖   found 2 problems, 0 warnings
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

⧗   input: refactor 1/2: Fix security policy version in tests to numbered string (#5549)
✖   subject may not be empty [subject-empty]
✖   type may not be empty [type-empty]

✖   found 2 problems, 0 warnings
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

⧗   input: bindings(rust): bump extended crates MSRV to 1.72.0 (#5534)
✖   type must be one of [build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test, opt, internal, tests] [type-enum]

✖   found 1 problems, 0 warnings
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

If there are no issues found, the command exits with no output.
The issues were:

  1. refactor 1/2: isn't quite right, if the 1/2 had been in () this would have passed...
  2. bindings(): is not a valid type.

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

@github-actions github-actions bot added the s2n-core team label Oct 31, 2025
@dougch dougch added the type/ci CodeBuild or CI related label Oct 31, 2025
@dougch dougch requested a review from maddeleine October 31, 2025 21:46
@dougch dougch marked this pull request as ready for review October 31, 2025 21:47
@dougch dougch requested review from boquan-fang and jmayclin and removed request for maddeleine November 3, 2025 23:58
@@ -0,0 +1,20 @@
module.exports = {
extends: ['@commitlint/config-conventional'],
rules: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why these particular rules? What does the [0] do? Can we just use someone else's config? Like what if we didn't "extend" the config-conventional

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already add this to s2n-quic repo: aws/s2n-quic#2881

The doc for this rule can be found in https://commitlint.js.org/reference/rules-configuration.html#rules-configuration.

Copy link
Contributor Author

@dougch dougch Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed like a reasonable starting point... here is the complete list

@commitlint/config-angular
@commitlint/config-conventional
@commitlint/config-lerna-scopes
@commitlint/config-nx-scopes
@commitlint/config-patternplate
@commitlint/config-workspace-scopes
conventional-changelog-lint-config-atom
conventional-changelog-lint-config-canonical

For subject-case 0 turns this off; I could not sus out a common pattern to past commit subject case; that isn't to say we shouldn't pick one, but I'm trying to ease us into this check. I can also add more doc links if that would help?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it seems like the most reasonable starting point is just their default? I understand that it might cause a bit of friction, but one of the benefits of this tooling is the consistency. If we hate it we can always change it 🤷

Some of the ones that we've specified seem less than ideal e.g. allowing test and tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,20 @@
module.exports = {
extends: ['@commitlint/config-conventional'],
rules: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,20 @@
module.exports = {
extends: ['@commitlint/config-conventional'],
rules: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it seems like the most reasonable starting point is just their default? I understand that it might cause a bit of friction, but one of the benefits of this tooling is the consistency. If we hate it we can always change it 🤷

Some of the ones that we've specified seem less than ideal e.g. allowing test and tests.

issuePrefixes: ['#']
}
},
ignores: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to ignore this? And what is actually getting ignored? Like conventional commits aren't enforced if the commit includes "Co-authored-by"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a line by line check, from my local testing. If someone adds a "recommended change" to your PR and you accept it, Co-authored-by gets added to the commit message. With-out this present, you would need to rebase and strip these out for this check to pass; example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, why does it fail that commit?

Co-authored-by: maddeleine <[email protected]>

Comment on lines +4 to +10
rules: {
'subject-case': [0],
'body-max-line-length': [0],
'footer-max-line-length': [0],
'type-enum': [2, 'always', [
'build', 'chore', 'ci', 'docs', 'feat', 'fix', 'perf', 'test'
]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rules: {
'subject-case': [0],
'body-max-line-length': [0],
'footer-max-line-length': [0],
'type-enum': [2, 'always', [
'build', 'chore', 'ci', 'docs', 'feat', 'fix', 'perf', 'test'
]]

Maybe I'm missing something, but can we just delete these and totally take everything from the predefined stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subject case is pretty pedantic; I'm in favor of strictness, but I'm trying to ease into this change and avoid the inevitable "why doesn't this pass?" question by relaxing it a bit.

@dougch
Copy link
Contributor Author

dougch commented Nov 5, 2025

Reworking in favor of a PR title checker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s2n-core team type/ci CodeBuild or CI related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants