CI: Add Pull Request format check #5000
Conversation
|
I personally think the format check should be separate from other checklist items, and then the rest should be one check (titled "PR Checklist" for example). Additionally, I'm not 100% sure on making this dependent on being on the master branch purely based on our past experience - users who did use the |
Alright
Would you suggest adding more information (maybe a link to a Git beginners guide) or just removing that check? |
|
A Git beginners guide, and one that provides a super basic guide on rebasing wouldn't be too bad - but in this case honestly removing the |
|
I'll leave it commented out for now then |
gxalpha
force-pushed
the
pr-format-check
branch
from
10f85c0
to
92a693b
Compare
|
As you mentioned I made the code format checks a mandatory step in the main workflow in my rework, that's where I'd put such a check as well (and also just use I'm not fully sold on the necessity of checking this via CI but not opposed to it if enough people feel it's worthwhile. |
There was a problem hiding this comment.
The regex pattern currently used in this PR does not cover all cases of commit messages that we would normally find acceptable and merge. Some of these are trivial to solve (add another character to the character class), some of these are not as easy. See my review comment for details.
I concur with @PatTheMav that I don't see the necessity of checking this in CI, though if it can gracefully handle things we'd normally permit without stopping an entire workflow, then I'm not terribly opposed to it either.
| pattern: '([a-zA-Z0-9\-_, ]*): (.{1,50})(\n){0,1}(\n.{1,72})*' | ||
| error: "The message of at least one of the commits not match the commit guidelines specified in CONTRIBUTING.rst" |
There was a problem hiding this comment.
There are occasions where the commit prefix contains a slash (see also one with a comma and slash), or the commit message subject contains the word "Revert" before the rest of the message follows in quotes, or the commit message has no prefix, or the commit message subject has more than 50 characters after the prefix because making it shorter makes it non-descriptive and our hard limit is 72 characters total. All of these would fail to pass this check.
However, this check would, as far as I can tell, pass a commit message like this:
: No prefix, but colon still here
.
As this is, I don't think I can approve this PR unless all of the cases I mentioned above are handled gracefully. Either the regex pattern needs to be adjusted, or a PR that fails this check needs to not hold up the rest of the workflow and simply be a warning. Even in the case of the warning, I would prefer that the regex pattern handle the cases I've outlined above.
|
|
||
| # - name: Check Branch | ||
| # if: (github.event_name == 'pull_request') && (success() || failure()) | ||
| # shell: bash | ||
| # run: | | ||
| # if [[ "${{ github.head_ref }}" == "master" ]]; then | ||
| # echo "Your code appears to be from the master branch. Make sure to use a separate branch for your pull requests.\n" >/dev/stderr | ||
| # exit 1 | ||
| # fi |
| push: | ||
| branches-ignore: [ master ] |
There was a problem hiding this comment.
| push: | |
| branches-ignore: [ master ] |
I would argue as this is designed for PRs entirely, this doesn't need to run on push at all.
|
#5248 looks (while more complicated) way better than this one. Closing. |
Description
Adds checks to make sure that the code is not on the master branch and that
the commits are properly formatted.
I wanted to cleanly keep all jobs one workflow, while not having endless skipped runs, thus the workflow
ignores pushes to the master branch.
As a side effect, the clang format will no longer run on that branch. However, this change also has been proposed in PatTheMavs cmake overhaul and I seemed to not get rejected yet, so I'll assume thats fine. Please correct me if I'm wrong.
Motivation and Context
An idea like this was discussed on the Discord a long time ago, but no one implemented it. Here you go.
How Has This Been Tested?
Tested on my fork:
Wrongly formatted commits: gxalpha#9
Correctly formatted commits: gxalpha#10
PR from master branch: gxalpha#12Check got removed, see belowTypes of changes
Checklist: