feat: Lint pr titles #6770

yihuiliao · 2024-07-25T23:05:53Z

The overall goal of this PR is to help improve the process of our release notes. Given that we use the titles of the squash commits on main to generate our changelog, improving the readability of our PR titles will hopefully allows us to spend less time manually fixing the names of each commit. Requiring the use of prefixes in our PR titles will eventually also allow us to automatically categorize the commits into the respective categories (like "Enhancement", "Fixes", "Docs", etc) but for now, will make it, in general, easier to organize our commits manually.

That said, I am aware that for cases where there is only one commit (not including merge commits), GitHub will use the commit message as the title for the squash commit. So this doesn't fix that issue. However, a majority of our PR's are multiple commits where the PR title will be used as the title for the squash commit so hopefully this will still help make release notes a bit easier.

For all outside contributors, this workflow will require our approval to run so that we only run this workflow on trusted code. If we approve the workflow and then the contributor pushes up new code, the workflow will require our reapproval to run again. Regardless if a contributor has contributed before, it will require our approval. This is one of the easiest ways to mitigate any bad actors from attempting to inject any malicious code.

Outside of the PR, I've written up a "PR Naming Guide" which I hope to publish on our Github wiki so that outsider contributors can reference it and understand these new requirements. I will also link that wiki in this PR so that if there is an error, you can click on the failed workflow and there will be a link to take you to the guide. (I have yet to finish writing this guide but that's the vision of it).

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

rspbot · 2024-07-25T23:25:03Z

Build successful! 🎉

yihuiliao · 2024-07-26T17:29:34Z

.github/actions/pr-linting/index.js

+    `🔎 Checking if the title of this PR "${title}" meets the requirements ...`
+  );
+
+  if ((/^\[?wip\]?/i).test(title)) {


Do we want to validate the PR title's if they are WIP's? I figured for now we just skip but wanted to see other people's opinions

yihuiliao · 2024-07-26T17:36:56Z

.github/workflows/lint-pr-titles.yaml

+on:
+  pull_request:
+    types: [opened, edited, reopened, synchronize]
+permissions:


Our repo grants read + write permissions to GITHUB_TOKEN which seemed to be the issue with GET_BUILD because we granted write access . I've updated the permissions here so that within this workflow they specifically are read only. Originally, I wanted to leave a comment if the PR title failed to validate, but that would require write access and since it didn't seem necessary, I've left it out.

I think it's fine, it'll show up in the job's log and it'd fail the PR

snowystinger

I think we should be prepared to fix people's title for them

snowystinger · 2024-08-07T03:57:14Z

.github/workflows/lint-pr-titles.yaml

+on:
+  pull_request:
+    types: [opened, edited, reopened, synchronize]
+permissions:


I think it's fine, it'll show up in the job's log and it'd fail the PR

snowystinger · 2024-08-07T03:58:13Z

.github/workflows/lint-pr-titles.yaml

+      - name: Use Node 18
+        uses: actions/setup-node@v4
+        with:
+          node-version: '18'


any reason not to use node 20 like the action?
node 20 will pass yarn install, it's just test-esm which gets hung up on it

no i can update it to use node 20 if it yarn install's fine. i just did node 18 since i know our codebase work with it

snowystinger · 2024-08-07T04:02:11Z

.github/actions/pr-linting/index.js

+    core.info('Success');
+  }
+  else {
+    core.info('PR title validation failed. Please read our PR naming guide to see how to correctly name your PR');


would be good to have a link ready to go for where to find the PR naming guide.
also, if it's not that long, maybe just inline it?

yeah i plan to update this with a link to a wiki page on github but that hasn't happened yet since i wanted to get the team's feedback on the contents before publishing anything

snowystinger

Please bring up in standup or in otters or setup a meeting to push for consensus on this.

snowystinger

Happy to try this out :)

LFDanLu

Noticed that the PR title check can take ~2 minutes with most of it being the install step. Maybe not an issue but what are we installing and can it be cached?

LFDanLu · 2024-08-22T20:40:59Z

.github/actions/pr-linting/index.js

+  if ((/^\[?wip\]?/i).test(title)) {
+    core.info('This PR is marked as a WIP. Skipping validation');
+  } 
+  else if ((/^(fix|feat|build|chore|docs|test|refactor|ci|localize|bump|revert)(\(([A-Za-z])\w+\))?:/gmi).test(title)) {


I'm a little concerned that some of these feel a bit similar (ci vs build) and/or perhaps too granular (test/refactor/bump/localize feel like they primary are updates done in tandem with larger fixes/features and may not be used too often). Happy to be overruled since I'm not familiar with how other repos handle these classifications, but it feels like maybe some of these could be placed under a generic prefix as part of its scope (e.g. fix(test) or chore(bump), etc) Maybe scope could be generalized and instead contain any supplementary information instead (e.g. fix(RAC, localize) Update ColorPicker intl strings to fix crash)

happy to start with a large set or a small set and see how it goes, can always update down the line

rspbot · 2024-08-22T20:43:48Z

Build successful! 🎉

The merge-base changed after approval.

rspbot · 2024-10-02T19:01:12Z

Build successful! 🎉

rspbot · 2024-10-02T19:55:25Z

Build successful! 🎉

rspbot · 2024-10-02T23:22:13Z

Build successful! 🎉

rspbot · 2024-10-02T23:35:06Z

Build successful! 🎉

rspbot · 2024-10-03T00:01:39Z

Build successful! 🎉

rspbot · 2024-10-03T01:32:06Z

Build successful! 🎉

yihuiliao · 2024-10-03T21:29:53Z

.github/workflows/lint-pr-titles.yaml

+            if [[ "${TITLE,,}" =~ ^\[?\(?wip\]?\)? ]]; then
+            echo "PR is marked as a WIP. Skipping validation."
+            exit 0
+            elif (echo "${TITLE,,}" | grep -P "^(fix|feat|build|chore|docs|test|refactor|ci|localize|bump|revert)(\(([A-Za-z0-9])+\))?:"); then


tbh my scripting knowledge is at a beginner level so there's probably a better way to handle this...but idea is there...

rspbot · 2024-10-03T21:35:42Z

Build successful! 🎉

LFDanLu

Happy to give this a try

snowystinger · 2024-10-13T23:30:18Z

How is this combining with the explorations that @devongovett was doing? I'd like to know the path we're taking forward

yihuiliao · 2024-10-14T19:13:01Z

How is this combining with the explorations that @devongovett was doing? I'd like to know the path we're taking forward

I guess both work on making our changelog more readable but they don't really interfere with each other. The way I see it for now is that this serves as an interim solution as Devon works on adding changesets.

This only runs on the PR after it is made on Github while what Devon is working on is ran (ideally) before pushing a PR. Each PR would contain it's own changeset which would get compiled later as the changelog once we do a release. With changesets, we wouldn't use the commit message on main to generate the changelog like we do now. The reason for linting PR titles right now is because it becomes the commit message on main. So, once the changesets gets released, this PR becomes a bit obsolete because what it solves now will be solved by changesets but in a different way. That said, our commit messages on main will still come from the title of the PR's so I don't think it hurts to lint them even if they won't be used to generate our changelog anymore. It also doesn't take much time to run now so that's not as much of a concern anymore

yihuiliao added 4 commits May 30, 2024 15:00

initialize github actions

c19b5ca

update linting code

7d035e9

update comment

69281d2

Merge branch 'main' into lint-pr-titles

0bf528d

yihuiliao commented Jul 26, 2024

View reviewed changes

adobe deleted a comment from rspbot Jul 26, 2024

snowystinger reviewed Aug 7, 2024

View reviewed changes

snowystinger approved these changes Aug 12, 2024

View reviewed changes

yihuiliao marked this pull request as ready for review August 22, 2024 20:27

Merge branch 'main' into lint-pr-titles

29d9454

yihuiliao changed the title ~~wip: Lint pr titles~~ feat: Lint pr titles Aug 22, 2024

snowystinger previously approved these changes Aug 22, 2024

View reviewed changes

LFDanLu reviewed Aug 22, 2024

View reviewed changes

reidbarber previously approved these changes Aug 22, 2024

View reviewed changes

devongovett force-pushed the main branch from fc5c8bd to e60fb42 Compare September 28, 2024 00:55

devongovett force-pushed the main branch from a79adcf to 3013156 Compare September 30, 2024 21:00

yihuiliao added 2 commits October 2, 2024 11:52

Merge branch 'main' into lint-pr-titles

d0401a4

update wip regex

39035ac

yihuiliao added 2 commits October 2, 2024 13:56

add link to pr naming guide

48365e0

cache yarn?

13640d3

yihuiliao changed the title ~~feat: Lint pr titles~~ feat: Lint pr titles test Oct 2, 2024

update frozen-lockfile to immutable for yarn 4

0d4ad50

try caching node modules

f4dd880

yihuiliao added the tech/dev/garage week label Oct 2, 2024

yihuiliao changed the title ~~feat: Lint pr titles test~~ feat: Lint pr titles Oct 2, 2024

yihuiliao changed the title ~~feat: Lint pr titles~~ feat: Lint pr titles test Oct 3, 2024

remove hardened mode, testing to see yarn install time

2be0a27

remove action, use workflow only

1182445

yihuiliao commented Oct 3, 2024

View reviewed changes

yihuiliao changed the title ~~feat: Lint pr titles test~~ feat: Lint pr titles Oct 3, 2024

LFDanLu approved these changes Oct 3, 2024

View reviewed changes

reidbarber approved these changes Oct 11, 2024

View reviewed changes

snowystinger approved these changes Oct 14, 2024

View reviewed changes

yihuiliao added this pull request to the merge queue Oct 15, 2024

Merged via the queue into main with commit ce87249 Oct 15, 2024
31 checks passed

yihuiliao deleted the lint-pr-titles branch October 15, 2024 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Lint pr titles #6770

feat: Lint pr titles #6770

yihuiliao commented Jul 25, 2024 •

edited

Loading

rspbot commented Jul 25, 2024

yihuiliao Jul 26, 2024

yihuiliao Jul 26, 2024

snowystinger Aug 7, 2024

snowystinger left a comment

snowystinger Aug 7, 2024

snowystinger Aug 7, 2024

yihuiliao Aug 9, 2024

snowystinger Aug 7, 2024

yihuiliao Aug 9, 2024

snowystinger left a comment

snowystinger left a comment

LFDanLu left a comment

LFDanLu Aug 22, 2024

snowystinger Aug 22, 2024

rspbot commented Aug 22, 2024

rspbot commented Oct 2, 2024

rspbot commented Oct 2, 2024

rspbot commented Oct 2, 2024

rspbot commented Oct 2, 2024

rspbot commented Oct 3, 2024

rspbot commented Oct 3, 2024

yihuiliao Oct 3, 2024

rspbot commented Oct 3, 2024

LFDanLu left a comment

snowystinger commented Oct 13, 2024

yihuiliao commented Oct 14, 2024 •

edited

Loading

feat: Lint pr titles #6770

feat: Lint pr titles #6770

Conversation

yihuiliao commented Jul 25, 2024 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

rspbot commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowystinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowystinger left a comment

Choose a reason for hiding this comment

snowystinger left a comment

Choose a reason for hiding this comment

LFDanLu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rspbot commented Aug 22, 2024

rspbot commented Oct 2, 2024

rspbot commented Oct 2, 2024

rspbot commented Oct 2, 2024

rspbot commented Oct 2, 2024

rspbot commented Oct 3, 2024

rspbot commented Oct 3, 2024

Choose a reason for hiding this comment

rspbot commented Oct 3, 2024

LFDanLu left a comment

Choose a reason for hiding this comment

snowystinger commented Oct 13, 2024

yihuiliao commented Oct 14, 2024 • edited Loading

yihuiliao commented Jul 25, 2024 •

edited

Loading

yihuiliao commented Oct 14, 2024 •

edited

Loading