search

amannn / action-semantic-pull-request

A GitHub Action that ensures that your PR title matches the Conventional Commits spec
MIT License
984 stars 124 forks source link

Validate PR commits as well #146

Open thiagomajesk opened 2 years ago

thiagomajesk commented 2 years ago

Hi! There's a common use-case for squash and merge workflows where you can keep all the PR commit history inside the final squashed commit as a comment. This is very useful for scenarios where we still want to keep a single commit going to trunk, but don't want to lose the development context. So, would you be willing to support linting the commits as well?

amannn commented 2 years ago

Hmm, I'm wondering if a tool like commitlint is in a better position to solve this? I could imagine this is especially useful in a precommit hook to avoid invalid commit messages in the first place.

What do you think?

thiagomajesk commented 2 years ago

Hi @amannn! Git hooks are not a reliable source, since a user can simply remove it to bypass the validation. Besides that, there are all sorts of implications on relying on the proper setup of a user machine to validate this kind of workflow. I'm looking for a way to enforce this on the server-side.

amannn commented 2 years ago

Sure, I understand. But maybe it would be a good idea to run commitlint in a Github action then?

This action is mostly concerned with validating the PR title, but we had to add validation for single commits due to a quirk in the way Github handles squash & merge.

thiagomajesk commented 2 years ago

I noticed that, but I think it would be a great addition, perhaps as a separate option? The problem is that there are many different actions that do one or the other. The problem is that they are often not as complete as your action, or don't have the same options to customize the behavior. So having to make all separate actions and maintaining different settings is kind of a PITA, to be honest.

wyardley commented 2 years ago

I would say if you're squash merging that it can actually get more messy / muddy the waters when the commits all have this format. That said, this feature could be useful for trunk-based projects where merge commits are used.

FWIW, at a previous role (before GHA launched), I'd used https://github.com/zeke/semantic-pull-requests which does support requiring it on commits only (for scenarios where you're doing merge commits or rebase merging), PR titles only, or both, and will also leave a hint of which types of merges will work for a given set of commits) (though IIRC, it didn't have the accomodations for GH's inconsistent behavior that this one does).

tstackhouse commented 2 years ago

This is actually recommended as an alternative to the project that @wyardley linked, which is also pertinent as that project is currently having issues. Validating the format of commits is also useful in a rebase and merge workflow, as mentioned above. Having this as an option would be helpful, though I'm also going to explore using a tool like commitlint to check the messages. We already use it on a githook, but as @thiagomajesk pointed out, git hooks can easily be bypassed by end-users, and having the action check the messages as well provided additional insurance.

thiagomajesk commented 2 years ago

I don't think I agree with @wyardley's affirmation. I'm actually already using a workflow based on squash merging and the problem is precisely to enforce that users keep consistency on naming commits (even if they'll be discarded at the end).

Ultimately I think it comes down to just validating the whole PR and leaving it to the end-user to decide which workflow works best for him, be it squash merge or creating a merge commit at the end.

Like @tstackhouse also pointed out, I can't stress enough how problematic is relying on git hooks for this... Besides that, since I already have to use https://github.com/upsidr/merge-gatekeeper to keep some sort of state regarding what's failed, I'm trying not to be wasteful with the number of jobs in the pipeline. If I can get an action that validates the whole PR it would be great.

@amannn you mentioned a quirk on Github's side, could you explain a little bit more?

amannn commented 2 years ago

Here's some background on the issue that GitHub has:

It's why we introduced the config options validateSingleCommit and validateSingleCommitMatchesPrTitle.

I see the point that pre-commit hooks are not entirely reliable. But what would the workflow be when you've got a bad commit message in a PR? Do you need to amend the message and push the update? If we decide that we want to validate for that, we should be able to provide a clear actionable error message for the user (possibly a link).

thiagomajesk commented 2 years ago

Hi @amannn, I see... Interestingly enough, I think Github's behavior is more consistent than Gitlab's; or at least I got accustomed to having single commits merged disregarding the PR's title (it's cool to have this option in the action thought). However, I have to admit that I don't understand how this affects validating the commits in the PR.

Here's the workflow I think works best:

image

Now, instead of well-formatted commit messages, imagine a lot of 'wips' in the history.

wyardley commented 2 years ago

This action already supports validating a single commit in the case where there’s only one. I think the request here is to be able to validate each commit of a PR with multiple commits against conventional commits, which is what I’m saying is (IMO) somewhat of an antipattern.

That is, if you’re squash merging the PR and using the default behavior of keeping the title as the commit and all the commits as a body, to me, it doesn’t make sense to have all the individual commits have the conventional commits style in the body of the commit message.

While I agree that GH’s inconsistent behavior with 1 or >1 commit when squash merging is not ideal, one downside of making it perfectly consistent is that it then becomes difficult to use the body of the commit message as it’s really intended.

thiagomajesk commented 2 years ago

@wyardley It's exactly what we've been discussing and that's why people suggested using git hooks as well. Don't worry about it being possibly an antipattern or not (which I strongly disagree with), it's a completely valid and real use case.

That is, if you’re squash merging the PR and using the default behavior of keeping the title as the commit and all the commits as a body, to me, it doesn’t make sense to have all the individual commits have the conventional commits style in the body of the commit message.

Having the commits in the body following the conventional commits style is a matter of taste nonetheless. But it also is really helpful to keep history consistent and encourage people to write better commit messages. It's virtually the same thing as adding a git hook, except it runs on the server. So much so that it shouldn't care what will happen to those commits at the end, the only concern is to have an option to validate commits and not care what will happen to them afterward.

tstackhouse commented 2 years ago

Setting aside if it is an antipattern or not, in my project we use rebase merges without squashing to preserve a linear history, and therefore need all commit messages to be in conventional format. I've worked around the limitations of this action by incorporating this action into my workflow: https://github.com/wagoid/commitlint-github-action Since I already use commitlint in my hooks, my config flows through and gets picked up just fine.

@thiagomajesk to your point about all the WIPs, that is precisely why rebasing and force pushes exist. You can clean up your commit history prior to merging so that the history makes more sense, breaking up and reorganizing your commits before they ever mage it into your main branch.

wyardley commented 2 years ago

in my project we use rebase merges without squashing to preserve a linear history

Agreed. This (or a merge commit into trunk when not using a multi branch gitflow type setup) would be really good reasons to have this functionality configurable here.

thiagomajesk commented 2 years ago

@thiagomajesk to your point about all the WIPs, that is precisely why rebasing and force pushes exist. You can clean up your commit history prior to merging so that the history makes more sense, breaking up and reorganizing your commits before they ever mage it into your main branch.

Exactly @tstackhouse! But again, we are talking about enforcing those rules. Validation is helpful for consistency purposes, after all, we use CI to prevent us from doing those kinds of silly 'mistakes' and enforce a specific workflow. Same thing about the PR title.

amannn commented 2 years ago

I have to say, personally I'm not really into validation on the commit-level. I've once worked on a project where this was enforced and I found a commit chain of "feat: Add button", "fix: Handle null pointer in click handler" quite confusing, since it provides the notion that a fix is contained in the PR, whereas the whole PR just represents a feature and the fix is related to the feature, so in perspective of the overall app there's no fix, just a feature.

That said, your situation might be different and require this kind of validation. So if someone feels strong about this feature, I'd be open to a contribution.

Some requirements I can think of:

  1. All config options that we currently allow are considered for commits too when the flag is enabled and they can be applied (e.g. subjectPattern, …).
  2. There should be error handling for options that can't work together – e.g. validateSingleCommit should not be set when the new flag is turned on.
  3. In case commit validation fails there should be helpful error messages that helps the commit author to resolve the issue.

IMHO a good way to notify the user about the validation would be just commenting on the PR.

I'd like to keep the noise of this action to a minimum, so in case you follow the provided configuration, you shouldn't hear anything from this action. Only when things go wrong there should be an error message and I think checks work really well for that.

thiagomajesk commented 2 years ago

I have to say, personally I'm not really into validation on the commit-level. I've once worked on a project where this was enforced and I found a commit chain of "feat: Add button", "fix: Handle null pointer in click handler" quite confusing, since it provides the notion that a fix is contained in the PR, whereas the whole PR just represents a feature and the fix is related to the feature, so in perspective of the overall app there's no fix, just a feature.

@amannn I completely agree, and I think this is true for the most part. My team and I don't particularly mix commit types in the same PR. However, when doing R&D, we are required to lift some of those preconceived notions and abstract what that PR means differently for our product. For instance, I might be working on a huge refactor that also contains some necessary fixes, and I have two options at the end a) rebase it and keep commits on the body; b) don't rebase and merge all commits to the trunk. This gives me a lot of control over what strategy works best for a specific use case while keeping a consistent history.

I'd like to keep the noise of this action to a minimum, so in case you follow the provided configuration, you shouldn't hear anything from this action. Only when things go wrong there should be an error message and I think checks work really well for that.

That's precisely what I meant. In case there are "problematic" commits in a PR, it should be displayed as a table or some report like that, but only if there's an actionable/ blocking problem.

Just to comment a little further on the requirements: I don't think that validateSingleCommit would be exclusive, they serve different purposes, and that's why commit validation should be done first. I might still want to only keep a single commit at the end with the previous messages in the body (of course the new flag should be false by default to avoid surprises). The rest I think is fairly reasonable.