Skip to content

Commit 0486797

Browse files
authored
ci: conventional commits aws#5458
Problem: PR titles and commit subjects are not always clear. Solution: - Enforce a simple convention for PR titles. - Require the "lint-commits" and "lint" jobs to succeed before running other CI jobs.
1 parent b304441 commit 0486797

File tree

3 files changed

+270
-38
lines changed

3 files changed

+270
-38
lines changed

.github/workflows/lintcommit.js

Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,200 @@
1+
// Checks that a PR title conforms to our custom flavor of "conventional commits"
2+
// (https://www.conventionalcommits.org/).
3+
//
4+
// To run self-tests, simply run this script:
5+
//
6+
// node lintcommit.js test
7+
//
8+
// TODO: "PR must describe Problem in a concise way, and Solution".
9+
// TODO: this script intentionally avoids github APIs so that it is locally-debuggable, but if those
10+
// are needed, use actions/github-script as described in: https://github.com/actions/github-script?tab=readme-ov-file#run-a-separate-file
11+
//
12+
13+
const fs = require('fs')
14+
// This script intentionally avoids github APIs so that:
15+
// 1. it is locally-debuggable
16+
// 2. the CI job is fast ("npm install" is slow)
17+
// But if we still want to use github API, we can keep it fast by using `actions/github-script` as
18+
// described in: https://github.com/actions/github-script?tab=readme-ov-file#run-a-separate-file
19+
//
20+
// const core = require('@actions/core')
21+
// const github = require('@actions/github')
22+
23+
const topics = new Set([
24+
'build',
25+
// Don't allow "chore" because it's over-used.
26+
// Instead, add a new topic if absolutely needed (if the existing ones can't possibly apply).
27+
// 'chore',
28+
'ci',
29+
'config',
30+
'docs',
31+
'feat',
32+
'fix',
33+
'perf',
34+
'refactor',
35+
'style',
36+
'telemetry',
37+
'test',
38+
'types',
39+
])
40+
41+
// TODO: Validate against this once we are satisfied with this list.
42+
const scopes = new Set([
43+
'amazonq',
44+
'core',
45+
'lambda',
46+
'logs',
47+
'redshift',
48+
'q-chat',
49+
'q-featuredev',
50+
'q-inlinechat',
51+
'q-transform',
52+
's3',
53+
'telemetry',
54+
'ui',
55+
])
56+
void scopes
57+
58+
/**
59+
* Checks that a pull request title, or commit message subject, follows the expected format:
60+
*
61+
* topic(scope): message
62+
*
63+
* Returns undefined if `title` is valid, else an error message.
64+
*/
65+
function validateTitle(title) {
66+
const parts = title.split(':')
67+
const subject = parts.slice(1).join(':').trim()
68+
69+
if (parts.length < 2) {
70+
return 'missing colon (:) char'
71+
}
72+
73+
const topicScope = parts[0]
74+
75+
if (topicScope.startsWith('revert')) {
76+
return validateTitle(subject)
77+
}
78+
79+
const [topic, scope] = topicScope.split(/\(([^)]+)\)$/)
80+
81+
if (/\s+/.test(topic)) {
82+
return `topic contains whitespace: "${topic}"`
83+
} else if (topic === 'chore') {
84+
return 'do not use "chore" as a topic. If the existing valid topics are insufficent, add a new topic to the `lintcommit.js` script.'
85+
} else if (!topics.has(topic)) {
86+
return `invalid topic "${topic}"`
87+
} else if (!scope && topicScope.includes('(')) {
88+
return `must be formatted like topic(scope):`
89+
} else if (scope && scope.length > 30) {
90+
return 'invalid scope (must be <=30 chars)'
91+
} else if (scope && /[^- a-z0-9]+/.test(scope)) {
92+
return `invalid scope (must be lowercase, ascii only): "${scope}"`
93+
} else if (subject.length === 0) {
94+
return 'empty subject'
95+
} else if (subject.length > 100) {
96+
return 'invalid subject (must be <=100 chars)'
97+
}
98+
99+
return undefined
100+
}
101+
102+
function run() {
103+
const eventData = JSON.parse(fs.readFileSync(process.env.GITHUB_EVENT_PATH, 'utf8'))
104+
const pullRequest = eventData.pull_request
105+
106+
// console.log(eventData)
107+
108+
if (!pullRequest) {
109+
console.info('No pull request found in the context')
110+
return
111+
}
112+
113+
const title = pullRequest.title
114+
115+
const failReason = validateTitle(title)
116+
const msg = failReason
117+
? `
118+
Pull request title does not match the [expected format](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#pull-request-title):
119+
120+
* Title: \`${title}\`
121+
* Reason: ${failReason}
122+
* Expected format: \`topic(scope): subject...\`
123+
* topic: one of (${Array.from(topics).join(', ')})
124+
* scope: lowercase, <30 chars
125+
* subject: must be <100 chars
126+
`
127+
: `Pull request title matches the [expected format](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#pull-request-title).`
128+
129+
if (process.env.GITHUB_STEP_SUMMARY) {
130+
fs.appendFileSync(process.env.GITHUB_STEP_SUMMARY, msg)
131+
}
132+
133+
if (failReason) {
134+
console.error(msg)
135+
process.exit(1)
136+
} else {
137+
console.info(msg)
138+
}
139+
}
140+
141+
function _test() {
142+
const tests = {
143+
' foo(scope): bar': 'topic contains whitespace: " foo"',
144+
'build: update build process': undefined,
145+
'chore: update dependencies':
146+
'do not use "chore" as a topic. If the existing valid topics are insufficent, add a new topic to the `lintcommit.js` script.',
147+
'ci: configure CI/CD': undefined,
148+
'config: update configuration files': undefined,
149+
'docs: update documentation': undefined,
150+
'feat(foo): add new feature': undefined,
151+
'feat(foo):': 'empty subject',
152+
'feat foo):': 'topic contains whitespace: "feat foo)"',
153+
'feat(foo)): sujet': 'invalid topic "feat(foo))"',
154+
'feat(foo: sujet': 'invalid topic "feat(foo"',
155+
'feat(q foo bar): bar':
156+
'do not use "chore" as a topic. If the existing valid topics are insufficent, add a new topic to the `lintcommit.js` script.',
157+
'feat(Q Foo Bar): bar': 'invalid scope (must be lowercase, ascii only): "Q Foo Bar"',
158+
'feat(scope):': 'empty subject',
159+
'feat(q foo bar): bar': undefined,
160+
'feat(foo): x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x x ':
161+
'invalid subject (must be <=100 chars)',
162+
'fix(a-b-c): resolve issue': undefined,
163+
'foo (scope): bar': 'topic contains whitespace: "foo "',
164+
'invalid title': 'missing colon (:) char',
165+
'perf: optimize performance': undefined,
166+
'refactor: improve code structure': undefined,
167+
'revert: feat: add new feature': undefined,
168+
'style: format code': undefined,
169+
'test: add new tests': undefined,
170+
'types: add type definitions': undefined,
171+
}
172+
173+
let passed = 0
174+
let failed = 0
175+
176+
for (const [title, expected] of Object.entries(tests)) {
177+
const result = validateTitle(title)
178+
if (result === expected) {
179+
console.log(`✅ Test passed for "${title}"`)
180+
passed++
181+
} else {
182+
console.log(`❌ Test failed for "${title}" (expected "${expected}", got "${result}")`)
183+
failed++
184+
}
185+
}
186+
187+
console.log(`\n${passed} tests passed, ${failed} tests failed`)
188+
}
189+
190+
function main() {
191+
const mode = process.argv[2]
192+
193+
if (mode === 'test') {
194+
_test()
195+
} else {
196+
run()
197+
}
198+
}
199+
200+
main()

.github/workflows/node.js.yml

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,40 @@ on:
1010
branches: [master, feature/*, mynah-dev]
1111

1212
jobs:
13+
lint-commits:
14+
runs-on: ubuntu-latest
15+
steps:
16+
- uses: actions/checkout@v4
17+
with:
18+
fetch-depth: 20
19+
- uses: actions/setup-node@v4
20+
with:
21+
node-version: '20'
22+
- name: Check PR title and commit message
23+
run: |
24+
node "$GITHUB_WORKSPACE/.github/workflows/lintcommit.js"
25+
26+
lint:
27+
needs: lint-commits
28+
runs-on: ubuntu-latest
29+
strategy:
30+
matrix:
31+
node-version: [16.x]
32+
vscode-version: [stable]
33+
env:
34+
NODE_OPTIONS: '--max-old-space-size=8192'
35+
steps:
36+
- uses: actions/checkout@v4
37+
- name: Use Node.js ${{ matrix.node-version }}
38+
uses: actions/setup-node@v4
39+
with:
40+
node-version: ${{ matrix.node-version }}
41+
- run: npm ci
42+
- run: npm run testCompile
43+
- run: npm run lint
44+
1345
macos:
46+
needs: lint-commits
1447
name: test macOS
1548
runs-on: macos-latest
1649
strategy:
@@ -80,6 +113,7 @@ jobs:
80113
token: ${{ secrets.CODECOV_TOKEN }}
81114

82115
web:
116+
needs: lint-commits
83117
name: test Web
84118
runs-on: ubuntu-latest
85119
strategy:
@@ -105,6 +139,7 @@ jobs:
105139
run: npm run testWeb
106140

107141
windows:
142+
needs: lint-commits
108143
name: test Windows
109144
runs-on: windows-2019
110145
strategy:
@@ -135,22 +170,3 @@ jobs:
135170
verbose: true
136171
file: ./coverage/lcov.info
137172
token: ${{ secrets.CODECOV_TOKEN }}
138-
139-
lint:
140-
name: Lint
141-
runs-on: ubuntu-latest
142-
strategy:
143-
matrix:
144-
node-version: [16.x]
145-
vscode-version: [stable]
146-
env:
147-
NODE_OPTIONS: '--max-old-space-size=8192'
148-
steps:
149-
- uses: actions/checkout@v4
150-
- name: Use Node.js ${{ matrix.node-version }}
151-
uses: actions/setup-node@v4
152-
with:
153-
node-version: ${{ matrix.node-version }}
154-
- run: npm ci
155-
- run: npm run testCompile
156-
- run: npm run lint

CONTRIBUTING.md

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -279,34 +279,50 @@ user's point of view.
279279
> - If there are multiple unrelated changes, run `npm run newChange` for each change.
280280
> - Include the feature that the change affects, Q, CodeWhisperer, etc.
281281
282-
### Commit messages
282+
### Pull request title
283283
284-
Generally your PR description should be a copy-paste of your commit message(s).
285-
If your PR description provides insight and context, that also should exist in
286-
the commit message. Source control (Git) is our source-of-truth, not GitHub.
284+
The title of your pull request must follow this format (checked by [lintcommit.js](.github/workflows/lintcommit.js)):
287285
288-
Follow these [commit message guidelines](https://cbea.ms/git-commit/):
286+
- format: `topic(scope): subject...`
287+
- topic: must be a valid topic (`build`, `ci`, `config`, `docs`, `feat`, `fix`, `perf`, `refactor`, `style`, `telemetry`, `test`, `types`)
288+
- see [lintcommit.js](.github/workflows/lintcommit.js))
289+
- "chore" is intentionally rejected because it tends to be over-used.
290+
- user-facing changes should always choose "feat" or "fix", and include a [changelog](#changelog) item.
291+
- scope: lowercase, <30 chars
292+
- subject: must be <100 chars
289293
290-
- Subject: single line up to 50-72 characters
291-
- Imperative voice ("Fix bug", not "Fixed"/"Fixes"/"Fixing").
292-
- Body: for non-trivial or uncommon changes, explain your motivation for the
293-
change and contrast your implementation with previous behavior.
294-
- Often you can save a _lot_ of words by using this simple template:
295-
```
296-
Problem: …
297-
Solution: …
298-
```
294+
### Pull request description
295+
296+
Your PR description should provide a brief "Problem" and "Solution" pair. This
297+
structure often gives much more clarity, more concisely, than a typical
298+
paragraph of explanation.
299+
300+
Problem:
301+
Foo does nothing when user clicks it.
302+
303+
Solution:
304+
- Listen to the click event.
305+
- Emit telemetry on success/failure.
299306
300-
A [good commit message](https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project)
301-
has a short subject line and unlimited detail in the body.
302307
[Good explanations](https://nav.al/explanations) are acts of creativity. The
303308
"tiny subject line" constraint reminds you to clarify the essence of the
304309
commit, and makes the log easy for humans to scan. The commit log is an
305310
artifact that will outlive most code.
306311
307-
Prefix the subject with `type(topic):` ([conventional
308-
commits](https://www.conventionalcommits.org/) format): this again helps humans
309-
(and scripts) scan and omit ranges of the history at a glance.
312+
### Commit messages
313+
314+
Source control (Git) is our source-of-truth, not GitHub. However since most PRs
315+
are squash-merged, it's most important that your [pull request description](#pull-request-description)
316+
is well-formed so that the merged commit has the relevant info.
317+
318+
If you expect your commits to be preserved ("regular merge"), then follow [these
319+
guidelines](https://cbea.ms/git-commit/):
320+
321+
- Subject: single line up to 50-72 characters
322+
- Imperative voice ("Fix bug", not "Fixed"/"Fixes"/"Fixing").
323+
- [Formatted as `topic(scope): subject...`](#pull-request-title).
324+
- Helps humans _and_ scripts scan and omit ranges of the history at a glance.
325+
- Body: describe the change as a [Problem/Solution pair](#pull-request-description).
310326
311327
## Tooling
312328

0 commit comments

Comments
 (0)