fix: Single commit validation does not factor in merge commits [#108] #131
Conversation
julianpoy
force-pushed
the
fix-validatesinglecommit
branch
from
9b0d626
to
cb3667e
Compare
There was a problem hiding this comment.
Thank you so much for looking into this again!
Lastly, I noticed that the test workflows included in this repository (the one titled current branch) don't use any of the additional options that this repo has to offer, such as the validateSingleCommit option. I would recommend that the current branch workflow test for this repository either include these options, or that there be multiple workflows defined - one for each option.
That's a fair point. There used to be errors when external contributors use the preview workflow, as environment variables were not available (see event triggers in the README). But I think Github has changed something here, as there's now a button for maintainers to manually trigger CI runs from external contributors.
So potentially this is no longer an issue. If you feel like it, it would be nice if you add a new workflow with the validateSingleCommit option as you've suggested!
|
Added workflow for validateSingleCommit option and used paginate iterator and a break statement to exit early when enough commits have been pulled to make a decision. See the example PR for validation that the new code works. |
|
|
||
| if (commits.length === 1) { | ||
| for await (const response of client.paginate.iterator( |
| // the PR title or a commit message for the squash commit message. | ||
| nonMergeCommits = commits.filter( | ||
| (commit) => !commit.commit.message.startsWith('Merge branch') | ||
| ); |
There was a problem hiding this comment.
Sorry, another minor nit: I think we don't need commits, but only nonMergeCommits – right? Now we're applying this filter on every iteration step.
Maybe something like this would be a bit simpler?
if (!response.data.commit.message.startsWith('Merge branch')) {
nonMergeCommits.push(response.data);
}There was a problem hiding this comment.
Personally think that's a micro optimization. I also find the existing solution cleaner, and gives us the ability to take actions on the whole list of commits in the future if need be.
If you'd like to switch it though, feel free. It's your repo so I respect the decision. Both should work just fine.
One note is that if you implement your example, you'll need to make sure to spread response.data (nonMergeCommits.push(...response.data).
There was a problem hiding this comment.
Ok, fair enough – let's go with your approach and avoid more changes & testing.
Thanks again!
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| with: | ||
| validateSingleCommit: true |
|
🎉 This PR is included in version 3.4.5 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
See julianpoy#2 for validation that this works.
Continuation of #127
The breaking problem with the original PR was that GitHub API returns a list of commits each with a property
commitinside which contains themessagethat we care about. When mapping overcommits => commit, I forgot to add an additional chain forcommits => commit.commit.Additionally, I realized that not having a comprehensive list of commits means that any user could potentially have a non semantic commit, a merge, and then a semantic commit, still resulting in the denial of the PR. Given that there could be any number of merge commits in between the original real commit and the final real commit, we need to pull all commits for the PR from the GitHub API. I've created a loop to fetch all commits from the GitHub API with a page cap for sanity (say if someone created a diff across two giant branches). This sanity limit could probably be lowered if desired (currently 100 pages).
I also noticed that validatePrTitle should check the nonMergeCommit that we found in the case that there is, in fact, only one commit, rather than any one of the commits in case the first commit returned is a merge commit (for some strange reason).
Lastly, I noticed that the test workflows included in this repository (the one titled current branch) don't use any of the additional options that this repo has to offer, such as the validateSingleCommit option. I would recommend that the current branch workflow test for this repository either include these options, or that there be multiple workflows defined - one for each option.