commitlint-github-action icon indicating copy to clipboard operation
commitlint-github-action copied to clipboard

feature request: predict squash-merge commit on target branch and lint it as well

Open SvenStaehs opened this issue 2 years ago • 7 comments

This action checks the commits included in a PR before the merge, and it checks the push to target branch after the merge, but it does not warn the user if their PR would cause commitlint violations when merged to the target.

This happened recently in my project: we allow only squash-merge of PRs, with "default to pull request title and commit details" as commit message, so the commit on target branch has nothing to do with the commits we check during PR validation. Of course there is nothing preventing the user from changing this default and entering whatever they wish, but commitlint-github-action could predict the default commit message on the target branch and run it through commitlint.

There are multiple ways the merge message on target branch can be generated:

  • In my scenario, the commit header would be "${{ github.event.pull_request.title }} (#${{ github.event.pull_request.number }})" and commit body would be a list of all commits (with their headers prefixed with "* ")
  • other options for default commit message include the body of the commit being the PR description instead of a list of commits, and a "default message" whatever github decides that is.
  • if rebase is allowed, there won't be a separate message at all (I believe)
  • there may be other defaults to check, e.g. Dependabot ignores the default and forces the PR description into the commit body (with quite long lines due to it including links...)

So I guess I mainly want to start a discussion here if these things should be handled by this action at all or not. I know about amann's action to check the title, but it does not actually perform a commitlint, especially it does not check the length at all (hence our recent issue that we ended up with a merge commit on main branch that commitlint complained about... At least it was still parsable by semantic-release, so it didn't break anything but the commitlint step.)

Alternatively, it would be nice if we could just provide arbitrary text to be linted. I tried generating an empty commit with the predicted message header, but during PR-triggered workflows the action reads the list of commits in the PR with octokit, so I have no way of adding my mock commit to that list. If I could provide a "text-to-lint", that would solve my issue. Or a "commit-hashes" list that is used instead of reading it from PR, maybe this is useful for other circumstances as well?

Other workarounds include:

  • running commitlint directly just to check the title
  • check the title with amann's action and check the length with a separate bash step but I would far prefer using the same action that will double-check the push to target branch so I know the configuration is identical (e.g. for the length check with bash I won't be trying to read the commitlint.rc and will just hardcode the value ;)).

SvenStaehs avatar Jul 19 '23 09:07 SvenStaehs

Hi @SvenStaehs! That's a nice suggestion, as you said we don't have actions that actually use commitlint on the PR title yet, so they won't work in 100% of the cases. I'll add support for the textToLint parameter 💪

wagoid avatar Jul 23 '23 10:07 wagoid

Thanks! I also remembered our GHES server has an option to install serverside hooks, maybe that's an option. They're difficult to configure and should be used sparingly only for things that cannot be caught by PR checks, but I think this qualifies for that, so I'll look into that as well. The textToLint parameter will surely be helpful for future workarounds either way 😊

SvenStaehs avatar Jul 24 '23 13:07 SvenStaehs

Looking for the same thing

@wagoid any update on that?

alldayalone avatar Mar 05 '24 09:03 alldayalone

Looking for the same thing too, do you accept PRs ?

Wicpar avatar Apr 15 '24 19:04 Wicpar

@Wicpar yes, I do! I was thinking we could make the action work with merge_group event, that way we can get this issue fixed too https://github.com/wagoid/commitlint-github-action/issues/757

wagoid avatar Apr 16 '24 12:04 wagoid

I finally opted on using the commitlint command directly, it works by getting the title param and piping it into the command. I suppose that's what should be tone to implement it here too.

Wicpar avatar Apr 16 '24 12:04 Wicpar

Hey @wagoid, was wondering if there was any progress regarding that? I'm the one created #757 btw, so would love to know if my issue somehow helped

YossiSaadi avatar Oct 13 '24 12:10 YossiSaadi

Hey @wagoid, I've forked created a PR #806 also tested it and it seem to work perfect :)

The PR: https://github.com/yossi-test-org/test-merge-queue/pull/24 image

with bad squashed commit: https://github.com/yossi-test-org/test-merge-queue/actions/runs/12221132350/job/34089796604 image

with good squashed commit: https://github.com/yossi-test-org/test-merge-queue/actions/runs/12221143821/job/34089822554 image

YossiSaadi avatar Dec 08 '24 11:12 YossiSaadi