-
Notifications
You must be signed in to change notification settings - Fork 19
Checking commit messages in CI #24
Comments
IMO, yes. We could verify line lengths, the blank line, presence of |
If verifying this is going to be automated can someone finally take a stab at automating the creation of all of it :) |
@mikeal you mean the creation of metadata? |
creating the metadata and squashing the commit, basically all the manual steps in the collaboration guide. |
That part was being discussed at nodejs/build#325 |
Examples of bad commit msgs we have in there now that keep on bothering me. git log --format='%s' \
| grep -Ev '^((:?[a-z0-9_]|\-|,|, )+): ([^A-Z]|[A-Z0-9]+ )|^\d{4}-\d{2}-\d{2},? (Version|io\.js)|^Revert|^Working on' \
| less Also the lack of PR-URL is a big problem, while we've made our tooling work around it, it makes the process a little less accurate and potentially error prone. We can't exactly expect PR-URL and Reviewed-By to exist on incoming PRs, at least initially, and usually those are only added later on. Perhaps just a commit hook that prints warnings to stderr (i.e. not too obtrusive). |
Right, commits don't generally have metadata when they are run through node-test-pull-request. |
@rvagg we could introduce a push hook for pushing to master or a release branch |
Yeah, nice, |
Closing here. This can go in build repo if we want to keep talking about it. (It has been proposed that this repo might be archived, which I think would be a good idea TBH.) |
Would this be useful?
We have some logic in https://ci.nodejs.org/job/node-push-merge-commit that already manipulates all commit messages in a PR. With the addition of a verification script for a given commit message (script contribution welcome), I think I could make this into a check for node-test-pull-request.
The text was updated successfully, but these errors were encountered: