-
Notifications
You must be signed in to change notification settings - Fork 281
(chore) O3-1109: Add a GitHub Action to check that PR titles include a version bump marker #1347
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
base: main
Are you sure you want to change the base?
Conversation
|
Hello @brandones ! |
|
Great, thanks @Twiineenock ! I'm going to change the title of this PR a few times just to test it out. I'll change it back when I'm done. |
|
@denniskigen @ibacher I'm of the opinion that |
|
"BREAKING: O3-1109: Add a GitHub Action to check that PR titles include a version bump marker" fails with It should pass. |
I'm on-board. I kind of dislike "refactor" anyway, since "refactoring" can describe both changing things in a way that doesn't affect the API and changing it in a way that does. |
brandones
left a comment
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.
Please remove support for the refactor label.
Please ensure that "BREAKING: O3-1109: Add a GitHub Action to check that PR titles include a version bump marker" passes.
.github/workflows/pr-title.yml
Outdated
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, edited, reopened, synchronize] |
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.
I don't think you need all of these. opened and edited should suffice.
.github/workflows/pr-title.yml
Outdated
| // Define valid patterns | ||
| const validPatterns = [ | ||
| // Pattern 1: (type) O3-XXXX: Description | ||
| /^\((docs|test|chore|fix|feat|refactor)\)\s+(O3-\d+):\s+.+$/, |
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.
I don't know that we necessarily want to restrict this to only tickets that start with O3-.
.github/workflows/pr-title.yml
Outdated
| /^\((docs|test|chore|fix|feat|refactor)\)\s+(O3-\d+):\s+.+$/, | ||
| // Pattern 2: (type) Description (no ticket) | ||
| /^\((docs|test|chore|fix|feat|refactor)\)\s+[^ ].+$/, | ||
| // Pattern 3: (BREAKING) O3-XXXX: Description |
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.
This isn't a format that should be allowed. BREAKING should never have parentheses (we want it to stand out).
.github/workflows/pr-title.yml
Outdated
| // Pattern 1: (type) O3-XXXX: Description | ||
| /^\((docs|test|chore|fix|feat|refactor)\)\s+(O3-\d+):\s+.+$/, | ||
| // Pattern 2: (type) Description (no ticket) | ||
| /^\((docs|test|chore|fix|feat|refactor)\)\s+[^ ].+$/, |
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.
You don't need two patterns for this:
/^\((?:docs|test|chore|fix|feat)\) (?:[A-Z][A-Z0-9]+-\d+: )?\S.*$/
Should match any valid PR. See this diagram.
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.
Note, too, that we don't actually want to use \s here since the only character allows should be a space. \S (the inverse of \s) is useful, though, as we want the first character of the message to be non-whitespace.
.github/workflows/pr-title.yml
Outdated
| // Pattern 3: (BREAKING) O3-XXXX: Description | ||
| /^\(BREAKING\)\s+(O3-\d+):\s+.+$/, | ||
| // Pattern 4: BREAKING: Description (no ticket) | ||
| /^BREAKING:\s+[^ ].+$/ |
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.
For BREAKING: the ticket always comes after the :, so BREAKING: O3-1234: Some change. Drawing on the stuff from my previous comment:
/^BREAKING: (?:[A-Z][A-Z0-9]+-\d+: )?\S.+/
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.
If you really wanted to be nuts:
/^(?:\((?:docs|test|chore|fix|feat)\)|BREAKING:) (?:[A-Z][A-Z0-9]+-\d+: )?\S.+$/
Not sure if that's a good idea or not.
.github/workflows/pr-title.yml
Outdated
| errorMessage += `• For BREAKING changes without ticket numbers, use "BREAKING: Description"\n`; | ||
| } else if (/^\([^)]+\)\s+O3-\d+[^:]/.test(title)) { | ||
| errorMessage += `• Ticket reference must be followed by a colon (O3-1234: Description)\n`; | ||
| } else if (/^[^(](docs|test|chore|fix|feat|refactor)/i.test(title)) { |
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.
No need to repeat the whole thing. We're just checking that the start of the line is a parenthesis:
| } else if (/^[^(](docs|test|chore|fix|feat|refactor)/i.test(title)) { | |
| } else if (/^[^(]/(/i.test(title)) { |
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.
But, of course, we actually want to check that there is at least one pair of parentheses.
.github/workflows/pr-title.yml
Outdated
| let errorMessage = `🚫 Invalid PR title format. Strict requirements:\n\n`; | ||
|
|
||
| // Check for common mistakes | ||
| if (/^BREAKING\s/i.test(title)) { |
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.
The error messages here look like they're meant to be somewhat additive, but since it's implemented as a series if if ... else statements, you'll only ever get messages from one block.
.github/workflows/pr-title.yml
Outdated
| errorMessage += `- "BREAKING: Description"\n\n`; | ||
| } | ||
|
|
||
| errorMessage += `🔍 Found these issues in your title: "${title}"\n\n`; |
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.
Shouldn't this come before we include the issues?
.github/workflows/pr-title.yml
Outdated
| errorMessage += `• Ticket reference must be followed by a colon (O3-1234: Description)\n`; | ||
| } else if (/^[^(](docs|test|chore|fix|feat|refactor)/i.test(title)) { | ||
| errorMessage += `• Type prefix must be in parentheses (e.g., "(feat)")\n`; | ||
| } else if (/^\([^)]+\):/.test(title)) { |
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.
We should have a check, independent of the check for parentheses that the value in the parentheses is one of our allowable types.
.github/workflows/pr-title.yml
Outdated
| } | ||
|
|
||
| // 2. Check ticket reference format | ||
| const ticketMatch = title.match(/(o3|03|O3)-\d+/); |
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.
It's definitely good to check for 03 and o3.
|
Hi @ibacher @brandones , |
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
Per [RFC-31](openmrs/openmrs-rfc-frontend#31), pull request titles must begin with a prefix that signals the type of version bump the change implies, such as:
(feat)for new features(fix)for bug fixes(docs)for documentation-only changes(test)for test-only updates(chore)for tooling or maintenance(refactor)for non-functional internal improvementsBREAKING:for backwards-incompatible changesOptionally, a ticket number may be included, but it must be correctly formatted to start with an uppercase
O3, followed by a dash and digits (e.g.O3-1234). Both lowercase "o" (o3) and numeric zero (03) as the prefix are considered invalid.✅ Valid PR Title Examples
❌ Invalid PR Title Examples
These examples are invalid and will be rejected:
Why we are using
amannn/action-semantic-pull-requestinstead ofactions/github-scriptWhile
amannn/action-semantic-pull-requestis great for enforcing the [Conventional Commits](https://www.conventionalcommits.org/) standard, it has limitations when it comes to fine-grained customization, especially:Despite customizing the
headerPattern, we hit a hard wall where titles like(feat)or(docs)—which are valid under our RFC—would be incorrectly rejected, and nuanced errors (like using03-1234) couldn't be detected flexibly.For maximum control and clarity, we moved to
[actions/github-script](https://github.com/actions/github-script)which gives us full programmatic power to:O3-123format)This shift allows OpenMRS to align tightly with our internal contribution standards while still respecting semantic principles.
Screenshots
The following screansshots are example checks
Valid PR Title

No version bump label fails

A zero instead of "O" fails

Related Issue
https://openmrs.atlassian.net/browse/O3-1109
Other
Screancasts
Check passed
passed.webm
Check failed
zero.webm