-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Utilize GitHub Actions to aid checking of commit message #2429
Utilize GitHub Actions to aid checking of commit message #2429
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2429 +/- ##
=======================================
Coverage 48.87% 48.87%
=======================================
Files 124 124
Lines 5238 5238
Branches 1109 1109
=======================================
Hits 2560 2560
Misses 2371 2371
Partials 307 307 ☔ View full report in Codecov by Sentry. |
Hi @KevinEyo1 why is the ci check passing when the commit message is empty? |
@yucheng11122017 It's a new bug that I'm working on right now, so will take abit more before the PR is ready. |
Fixed all bugs and functionally tested on GitHub. Currently only runs on pull_request trigger. Should it be run on push trigger as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to see that you were able to fix the bug caused by the script trying to interpolate the punctuation marks!
I think it makes sense to merge this PR first. Running it to check if there is a PR message when we merge the PR in can be a separate PR (tiny PRs are easier to review!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I think one thing we will have to be careful about is from now on that currently this script relies on
**Proposed commit message: (wrap lines at 72 characters)**
and **Checklist:** :ballot_box_with_check:
within the PR template to get the commit message.
If we change the PR template, or if accidentally edits the PR template when making a PR, this check will fail.
So if we change the PR template, we will have to update this script accordingly
This PR can definitely be expanded in the future for example,
- checking if the commit message is wrapped at 72 characters
- checking for other empty fields
- checking if checkboxes are ticked
What is the purpose of this pull request?
Overview of changes:
#2140
Added a job in a new workflow
pr-message-reminder.yml
for checking PR description for having a proposed commit message.Added a script
process_message.py
to aid in processing of PR description body.Anything you'd like to highlight/discuss:
Created a separate workflow instead of adding new job in
ci.yml
so as to avoid running all jobs anytime PR description body is edited, since only the new job needs to be run. Can keep within same workflow but would be more messy with many if statements.Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
GitHub Actions: add checking of proposed commit message
There is no checking of proposed commit message presence.
PR authors may forget to include proposed commit message.
Adding a check using GitHub Actions will help remind and ensure
that authors don't miss out on filling in the commit message.
Let's add a job to the a new workflow, pr-message-reminder.yml
file to help automate checking and reminding of filling in the
proposed commit message for each PR.
This approach automates the process, without having to have
other users check and remind PR authors themselves. Adding the
new job to a new workflow will allow greater control of job triggers
while maintaining clean code.
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major
,r.Minor
,r.Patch
.Breaking change release note preparation (if applicable):