ci: drop external action for checking PR title format#380
Conversation
james-garner-canonical
changed the title
james-garner-canonical
changed the title
james-garner-canonical
changed the title
james-garner-canonical
force-pushed
the
26-03+ci+roll-our-own-semantic-commit-names
branch
from
00c687c to
6a7c739
Compare
james-garner-canonical
changed the title
tonyandrewmeyer
left a comment
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
I really like this idea, thanks!
I tried this out locally on a few variations and it seems fine to me (a couple of small suggestions based on that). I haven't tried it in actual CI, but I think I understand what it would look like and it seems reasonable to me.
| match = _PATTERN.match(title) | ||
| if not match: | ||
| print( | ||
| f'PR title does not follow Conventional Commits format.\n' |
There was a problem hiding this comment.
Maybe link them to the relevant doc that explains contributing?
There was a problem hiding this comment.
Good call, I've added this to both error branches.
| if not match: | ||
| print( | ||
| f'PR title does not follow Conventional Commits format.\n' | ||
| f'Expected: <type>[(<scope>)][!]: <description>\n' |
There was a problem hiding this comment.
If you use, for example, Feat instead of feat, you end up here, since it doesn't match [a-z], and the error is a bit confusing, since type doesn't really imply lower-case.
What about making it [A-Za-z]? If you use Feat then you'll get the error that it's not a valid type, but the error message will be more obvious, because you'll see feat in the list.
There was a problem hiding this comment.
Good idea, done, thanks.
We currently pull in an external dependency, *and* use the `pull_request_target` event (it doesn't touch code so should be safe, but even so), just to validate the PR titles are conventional commits. I don't think there are enough PRs from external contributors to make this worthwhile. Even in a repo that does have the enforcement and has external contributions, like canonical/operator, I've seen most people just leave the title as-is and have fixed it for them. I think doing that in this repo is perfectly reasonable; we enforce this though doing it ourselves rather than automation. If it does end up being too much work, then a locally made check, like in canonical/charmlibs#380, seems a safer choice than bringing in something external just for this.
2 participants
This PR drops the use of an external, third-party action for checking PR title format, replacing it with a small, locally-defined Python script. This has the added security benefit of letting us run this check in the safer
pull_requestcontext, rather than onpull_request_target, which the action apparently required for creating a commit status.I'm proposing the same approach for
operatorin canonical/operator#2388