Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
SvenStaehs opened this issue Jul 19, 2023 · 6 comments

Comments

@SvenStaehs
Copy link

SvenStaehs commented Jul 19, 2023

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 ;)).
@wagoid
Copy link
Owner

wagoid commented Jul 23, 2023

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 💪

@SvenStaehs
Copy link
Author

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 😊

@alldayalone
Copy link

Looking for the same thing

@wagoid any update on that?

@Wicpar
Copy link

Wicpar commented Apr 15, 2024

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

@wagoid
Copy link
Owner

wagoid commented Apr 16, 2024

@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 #757

@Wicpar
Copy link

Wicpar commented Apr 16, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants