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
Validate PR commits as well #146
Comments
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? |
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. |
Sure, I understand. But maybe it would be a good idea to run 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. |
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. |
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). |
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 |
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... @amannn you mentioned a quirk on Github's side, could you explain a little bit more? |
Here's some background on the issue that GitHub has:
It's why we introduced the config options 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). |
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:
Now, instead of well-formatted commit messages, imagine a lot of 'wips' in the history. |
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. |
@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.
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. |
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. |
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. |
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. |
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:
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. |
@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
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 |
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?
The text was updated successfully, but these errors were encountered: