Skip to content

FINERACT-2462: Add github action to enforce one commit per user in a PR #5437

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.

Sign up for GitHub

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

Open
edk12564 wants to merge 1 commit into
base: develop
Choose a base branch
from
+57 −0
Open

FINERACT-2462: Add github action to enforce one commit per user in a PR #5437

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions .github/workflows/pr-one-commit-per-user-check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
name: Fineract PR One Commit Per User Check


on:
Copy link
Contributor

@adamsaghy adamsaghy Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be below no?

on:
  pull_request:
    types: [opened, synchronize, reopened]

Copy link
Contributor Author

@edk12564 edk12564 Feb 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi adam! Thank you for the review. I took a look again, and realized I was responding to the wrong thing. I thought you were referring to the the types, not the pull_request_target.

I think pull_request_target was used here because it allows us to run this check in the main repo's context, which me and Aman figured was okay considered no foreign code is actually being executed. This would allow us to run the check without needing someone to approve the test first.

More on this here: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request_target. Do you think this is okay, or is it more policy to make sure everything is approved first?

Copy link
Contributor

@adamsaghy adamsaghy Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is this newly added github actions were not executed on this PR which means it is targeting the wrong thing, no?

Copy link
Contributor Author

@edk12564 edk12564 Feb 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean with the target! To test this out, I did some investigating on my local fork where I submitted a PR into my own fork's develop branch.

My setup is: the develop branch has pr-one-commit-per-user.yml and the PR branch does not. I first tested with multiple commits at once, and it properly failed. I also tested the target by squashing my commits in the PR, and the test passed. From the results, it seems to properly target the PR branch, but runs the pull_request_target scripts that live in the develop branch. Some pictures below.

no script in PR:
Screenshot 2026-02-19 at 9 33 10 AM

failed test:
Screenshot 2026-02-19 at 9 47 47 AM

success test:
Screenshot 2026-02-19 at 10 25 21 AM

I think based on this, this one particular PR will not have checks applied. However, if it is merged into the develop branch, any future PR will still be checked, and target the actual PR branch, and not the apache/fineract repo. However, the problem of running without workflow permissions isn't a big issue, so I think we can definitely go either way. Please let me know what you think!

Copy link
Contributor

@adamsaghy adamsaghy Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running this workflow on the target branch is incorrect. You should use the one i shared, imho:

on:
  pull_request:
    types: [opened, synchronize, reopened]

Copy link
Contributor Author

@edk12564 edk12564 Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! I respect your experience here. Just made the change now!

pull_request:
types: [opened, reopened, synchronize]


permissions:
pull-requests: write


jobs:
verify-commits:
name: Validate One Commit Per User
runs-on: ubuntu-latest
timeout-minutes: 1
steps:
- name: Verify Commit Policy
id: check
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
commits=$(gh api "repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/commits") || { echo "::error::GitHub API request failed"; exit 1; }
if echo "$commits" | jq -e '.[] | select(.author == null)' > /dev/null; then
echo "null_authors=true" >> $GITHUB_OUTPUT
echo "::error::Some commits have a git email that is not linked to a GitHub account. Please ensure your git email matches one of your GitHub Account emails.\n\nPlease also squash your commits to prevent this message again."
exit 1
fi
user_ids=$(echo "$commits" | jq -r '.[] | select(.author.type != "Bot") | .author.id')
if echo "$user_ids" | sort | uniq -d | grep -q .; then
echo "multiple_commits=true" >> $GITHUB_OUTPUT
echo "::error::Multiple commits from the same author have been detected."
exit 1
fi
echo "Success: Each author has exactly one commit."
- name: Comment on PR
if: failure()
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Contributor

@Aman-Mittal Aman-Mittal Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is token is necessary in this?

Copy link
Contributor Author

@edk12564 edk12564 Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding that Github token isn't the same one we use to authenticate our accounts. It's a separate token created per workflow for using the Github API calls in your actions. I believe it is required to access .author.id.

More on this here:
https://docs.github.com/en/actions/tutorials/authenticate-with-github_token

Copy link
Contributor

@Aman-Mittal Aman-Mittal Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems Ok, Can you also check the workflow of stale.yml, i really like the way it checks prs. This will not run without workflow approval and stale is a cron based job.

Copy link
Contributor Author

@edk12564 edk12564 Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look now!

Copy link
Contributor Author

@edk12564 edk12564 Feb 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a bit of research, and I agree with your view. The Stale workflow has a lot of merits, and I will look into adding to what we have. To summarize though, this is what I found.

1 - The current implementation needs permissions for it to run properly. Stale.yml uses the main repo's context, which allows it to run without manual approval.

On this note, I found something we could use. If we use pull_request_target instead of pull_request, we could run our workflows with base repo permissions. I am reading about it here: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request_target.

The main issue for this seems to be that this raises security concerns about running code without prior approval. But I think this might be fine considering we are only looking at the commit repository, PR number, and userIds. We don't actually execute any foreign code. Let me know what you think on this one!

2 - I also see what you mean about running a cronjob. I think this is a good idea as well. However, if we want to run on a cronjob to handle older PRs, the issue is that we probably shouldn't spam comments in case of users abandoning their PR. I think we should implement something like checking the most recent comment, or maybe we won't produce warnings if the no one has committed in the past day or two? I will take a look and let you know what our options are here.

What do you think on those points?

Copy link
Contributor Author

@edk12564 edk12564 Feb 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aman-Mittal Hey Aman! Thanks for posting that point. It was a direction I hadn't thought of. I agree that if the direction of the project is to sign all our commits to prove contribution, it's a concern that we can only sign one person. I also brought up the point of Github not recognizing contribution unless it's a commit or co-authorship as well. Let's see what everyone says!

Copy link
Contributor

@adamsaghy adamsaghy Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think we need cron job. It will be executed alongside all other checks. When something got changed, it will be re-run. I see no issue here.

I think we are fine to have multiple commits if it came from different authors. The check should catch and handle the situation where:

Commit by Author A  -> So far good
Commit by Author A -> Nope, it's a duplication!

or

Commit by Author A -> So far good
Commit by Author B -> So far good
Commit by Author A -> Nope, it's a duplication!

Copy link
Contributor Author

@edk12564 edk12564 Feb 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi adam! Really sorry about this but I made a post in the dev mail list before I read this comment. That's completely my bad.

To summarize the post, I wrote on a potential improvement to One Commit per PR that might allow us to keep full credit for everyone involved. We can add co-contributors to Co-Authored-By and also the trailer they performed work in, like Designed-By or Tested-By. This gives them full contribution in Github's eyes without having multiple users in a commit. But the tradeoff is still that each person cannot sign their commit, like they can in One Commit per User as Aman mentioned.

Would you still like One Commit Per User here given the new information? I will say that the implementation works either way!

Copy link
Contributor

@adamsaghy adamsaghy Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have read your recommendations on the mail list. Thank you for sharing there. Hopefully others will chime in. I am just 1 voice from the many ;)

Copy link
Contributor Author

@edk12564 edk12564 Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me! Thank you sir 👍

run: |
if [ "${{ steps.check.outputs.null_authors }}" == "true" ]; then
gh pr comment ${{ github.event.pull_request.number }} --repo ${{ github.repository }} --body \
$'**One Commit Per User Check Failed**\n\nSome committers have a git email that does not match their GitHub account. Please ensure your git email matches one of your GitHub Account emails. Please also squash your commits to prevent this message again.'
fi
if [ "${{ steps.check.outputs.multiple_commits }}" == "true" ]; then
gh pr comment ${{ github.event.pull_request.number }} --repo ${{ github.repository }} --body \
$'**One Commit Per User Check Failed**\n\nEach user may only have one commit per PR. Please squash your commits.'
fi