Commit messages do not have expected expression

[eccles]

We use this action to ensure that the link to an Azure issue is present in the commit message. However there is an easy way to bypass the check:

submit PR with commit message that does not have the AB#NNNN in the commit message
the verify link issue check fails
developer edits the github commit message in the Github UI
verify link issue now PASSES.
dev merges the change.
the commit message on main branch does NOT contain the AB#NNNN link - not good

Please add a setting that ignores the message that is displayed in the Github UI as this seems to be a copy of the commit msg and not the actual commit message.

Our current github action is this:

name: "Verify Linked Task"

on: [pull_request]

jobs:
  check_pr:
    runs-on: ubuntu-latest
    steps:
      - name: Check PR
        uses: JJ/github-pr-contains-action@releases/v11
        with:
          github-token: ${{github.token}}
          bodyContains: "AB#\\d{4,5}"

[JJ]

Sorry, I am not sure I understand this issue. What I'm getting is that you can apparently edit the commit message using GitHub, something that maybe does not get through the actual commit message in the repository; and what you're saying here is that that github version of the message is the one that's picked up, as opposed to the version of the commit that's in the log, is that correct?

I don't know if that's the case, but it the PR body is created from a commit message, that's what's checked, not the original commit message; is that what you're pointing at?

[eccles]

Yeah - this is a little confusing and took a bit of digging.

My requirement is that the commit message that gets merged to the default branch must have the expression specified in the github action. whilst this code in this repo does fail if the required expression is not in the commit msg it PASSES if the user edits the first or top message of the PR.

So what I would like is for the commit msg to be checked and not the body (if that is different...)

[eccles]

instead of bodyContains have commitContains ?

[JJ]

But there are a couple of issues I can think of here. Main one is that if the check fails, changing the commit message will require force push, which is kinda frowned upon in most organizations; that, or closing the PR and opening it again from another branch; either way, drastic, non-coding action on the part of the user. The other issue is related: you don't know in advance if that commit is going to get into the history. You might edit the PR body and then squash commit, and you are going to be as good as new. So I see that as a matter of merging policy, more than something you can solve at the workflow level. If I'm not wrong (but might be) you can force squash commits in the GitHub configuration. A meet-me-in-the-middle solution which might be implemented is to force the PR body to refer to an existing and open issue. But a good technical solution needs a few tweaks (checking the issue status via the GitHub API), and it need not be part of this specific github action; it's relatively likely it exists, too... So a meet-me-in-the-middle-of-the-meet-me-in-the-middle solution might be to simply force a #\d+ regex in the body. Will not be checked, might easily be faked, but it will work in most cases, and can be done with the existing code. I can document that hack if it looks like a suitable (albeit ephemeral) solution to this issue.

[JJ]

Come to think of it, a feasible alternative would be to include a shouldIncludeIssue directive, which would be a shortcut to checking if this regex matches. What do you think?

[eccles]

Hmm - not sure about your objections to force push - it is quite acceptable on feature branches and is forbidden on the default branch. There is no feasible way of changing the commit message without it.

[eccles]

if force push is of concern then check if any commit contains the desired regex and the dev can push a fixup commit.

[eccles]

We quite often squash many commits together prior to rebasing if required and then force push to a feature branch.

[eccles]

Come to think of it, a feasible alternative would be to include a shouldIncludeIssue directive, which would be a shortcut to checking if this regex matches. What do you think?

seems a nice enhancement

[eccles]

So I am running some tests at the moment and if I push a commit without the regex the action faisl as expected.

If I then add the tag and do a force push the action still fails even though the commit msg is now correct.

[JJ]

Yea, it should. It's checking the pull request body, the commit message is not checked. As I say, checking every commit message would require some heavy lifting, retrieving every one of them using the Github API; additionally, it's going to need to check every commit every time, because actions are not good at all at keeping state (well, you can generate an artifact that saves the last commit that was checked...), and at that point, it does not really look as something an action focused on pull requests should do, because pull requests are not going to go into main automatically; it's rather something geared to work with pushes to the main branch, so it looks like a different kind of tool, not something that can be grafted onto this specific action. Also, it looks like something that would work better in closed source systems, which is something that I opted out of in this action relatively early (through the need of specific tokens, mainly), and heavy lifting with closed source means it will be costly for whoever runs the action. Besides, it's something that can be more easily done by having every dev in the organization install some local hook framework like pre-commit. That will reject commit messages if they don't follow some specific pattern, and it can even be connected to the local Jira or Github setup so that it effectively runs some checks on the specific issue that's being worked on. IIRC, Atlassian has something like that too for their suite of tools Jira-BitBucket.

Long story short: checking every commit in a PR (or in a push) for anything is costly, not trivial in programming terms, and certainly something that would be better handled by itself, outside this action.

[eccles]

ah ok - I will have to look elsewhere then - thanks for your help

[JJ]

Rereading the conversation, I don't think it can easily be done within the current framework. So closing for the time being; please reopen if you feel this is something that might add some value to your repos in the future.