CondeNast / conventional-pull-request-action

Lint pull requests with the conventional commit spec, for a clean and conventional commit history
Apache License 2.0
15 stars 9 forks source link

feat: Add ignoreCommits config value to skip single commit linting when unnecessary #18

Closed benhodgson87 closed 1 year ago

benhodgson87 commented 2 years ago

Fixes #17

For repos where the strategy is to squash merge all commits to a branch, and Github is set up to use the PR title as the merge commit title, this action currently always lints single commits, even if their content is irrelevant.

This PR adds a new config value, ignoreCommits, which forces the tool to always lint the title of the PR, and ignore the validity of any commits that may be made.

This allows users who merge commit to still use commitTitleMatch for match checking, but users who squash with the title to completely disable these checks.

Review Checklist

mattbedell commented 2 years ago

I'm hesitant to add a setting that allows the pre-populated merge commit message to be incorrect. In the linked issue, would changing this line to lint only the commitMessageSubject, fix the bug you encountered?

benhodgson87 commented 2 years ago

I'm hesitant to add a setting that allows the pre-populated merge commit message to be incorrect. In the linked issue, would changing this line to lint only the commitMessageSubject, fix the bug you encountered?

The issue here is this still requires the first commit message to conform to (for us) arbitrary linting requirements (eg. requiring users to mark their first commit with the type). For our use case - we just need the squashed PR title to include the type for semantic release versioning.

As an example of our problem with this, in some cases we have situations where engineers (and often non-engineers making small copy changes to markdown through the Github web UI) will forget to add the type to their first commit, as we traditionally mark our PR types through branch naming and other projects internally don't yet follow conventional commits, which then requires them to work out what's gone wrong, amend the commit message, and push it up just to pass linting, even if the PR title is actually perfectly correct. In cases where this happens with copy changes through the web UI it usually requires the approving codeowner to pull the branch, amend the commit, and push it up to pass, only for that commit to be discarded at merge.

I think a well documented escape hatch here does make sense, but that said we've been using the fork within our repo to get this functionality and it's been working well enough, I realise this may be an 'us' problem so I'm happy to continue running from that and close this if the use case doesn't fit the scope here.

mattbedell commented 1 year ago

@benhodgson87 makes sense, apologies for the long turn around time