Add GitHub action to check PR for formatted title #6991
Conversation
| const issue_number = context.payload.pull_request.number; | ||
| const owner = context.repo.owner; | ||
| const repo = context.repo.repo; | ||
| let body = ''; |
There was a problem hiding this comment.
This can be defined within the first if. We don't use it outside of the if cases below.
There was a problem hiding this comment.
I'm not sure what you mean, if it's defined in the first if then it's not available in the other cases. Maybe I'm misunderstanding your comment?
There was a problem hiding this comment.
Apologies for not being very clear. I couldn't see body being used outside of the if case. It can be moved inside the first if I think. Like this:
if (!regex.test(title)) {
let body = '';
if (regexNoSpace.test(title)) {
body = 'Your PR title is missing a space after the keyword. Please update it to: [Add | Remove | Update | Fix] <Title>. For example: Remove Old feature';
} else if (regexLowerCase.test(title)) {
body = 'The keyword in your PR title is not capitalized. Please update it to: [Add | Remove | Update | Fix] <Title>. For example: Update Existing feature';
} else {
body = 'Your PR does not have a [Add | Remove | Update | Fix] <Title>. Please add one to help streamline our changelog. For example: Fix Bug in feature';
}
github.issues.createComment({ issue_number, owner, repo, body });
}
There was a problem hiding this comment.
That's true, though I have a habit of declaring all variables outside of conditionals as it reduces cognitive load to declare everything you're going to work with ahead of the logic.
shalabhms
changed the title
| } else { | ||
| body = 'Your PR does not have a [Add | Remove | Update | Fix] <Title>. Please add one to help streamline our changelog. For example: Fix Bug in feature'; | ||
| } | ||
| github.issues.createComment({ issue_number, owner, repo, body }); |
There was a problem hiding this comment.
In addition to adding a comment can we also fail this action? If the action still succeeds we probably can't count on a comment enforcing this behavior.
There was a problem hiding this comment.
I am of the philosophy to warn first and move to blocking if warning doesn't yield results. Several engineers also expressed concern at blocks causing friction, we can always add blocking in the near future if the comments alone don't work.
There was a problem hiding this comment.
If we're not going to block on the title, this is a recommendation and not a requirement. If we want consistency in the PR titles, this should be a requirement. If it's a requirement recommendation, what's the ask for reviewers? Ignore the title and approve anyway? If it's anything else, where the reviewer asks the contributor to update the title, then this needs to be a requirement.
There was a problem hiding this comment.
I agree with making it a requirement (can be in a quick follow up PR if we want to test this first). I'd also prefer if we expand the keywords to allow verbing (adding/removing) which is very commonly used by many developers.
Signed-off-by: Sylvain Niles <[email protected]>
Signed-off-by: Sylvain Niles <[email protected]>
sylvainsf
force-pushed
the
sylvainsf-pr-format-check
branch
from
9e669a7 to
2d0b306
Compare
sylvainsf
had a problem deploying
to
functional-tests
GitHub Actions
Failure
|
Can we also update the contribution documentation to note that PR titles should follow a specific format now? If we want to enforce consistent titles we should make sure we document it. |
sylvainsf
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
|
@AaronCrawfis Yes, I'll add that when I add this bot to the documents repo. |
We should update this repo as well here: https://github.com/radius-project/radius/tree/main/docs/contributing/contributing-pull-requests |
sylvainsf
had a problem deploying
to
functional-tests
GitHub Actions
Failure
|
This pull request has been automatically marked as stale because it has been inactive for 90 days. Remove stale label or comment or this PR will be closed in 7 days. |
Description
Checks incoming PRs for nicely formatted title.
Type of change
Auto-generated summary
copilot:all