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

[edk12564]

Description

JIRA: FINERACT-2462

Implemented a Github Action that runs on PR request changes, opens, and closes.

The Github Action checks for:

1. Does each user have a single PR associated with their Github account for that PR?
2. Does each user have a valid git email configured to match their Github email?

The action on failure is to comment in the PR thread with the type of error, as well as show in the error’s logs.

The feature allows for multiple users per PR, but only 1 commit per user.

Edge Cases and Behavior

Test Cases I have tried:

• If git email does not match any emails registered for the github account, the check will fail. This is because they will have null values for author.id. The author fields are required to differentiate users, and make sure they are not bots.
• If there are multiple commits associated with an account, the check will fail.
• If all goes well, there will be a successful check and a pass.
• When a user has a misconfigured git account, and pushes their code, they will get the error that one of the committers have their git email misconfigured. If they fix and recommit without squashing, this issue will still persist since the old commit is still there. Need to recommend a squash as well to remove the commit history. Fixed now.

Edge Cases covered in code:

• If a PR is made with only bots, like from another Github action, the test should pass.
• If Github API is down, the test should fail.
• If there are both bots and users, only the users commits should be looked at.

Failure Path (misconfigured git email):

Failure Path (multiple commits per user):

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[Aman-Mittal]

Is token is necessary in this?

[edk12564]

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

[Aman-Mittal]

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.

[edk12564]

I'll take a look now!

[edk12564]

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?

[edk12564]

Hello @Aman-Mittal , Ashar Khan would like to work on this ticket as well. But before that, should we go ahead with the Cron based feature as well? Because I believe otherwise, the work is completed after my next commit. Once I can figure out these GPG keys.

[Aman-Mittal]

@edk12564 You are free to colaborate with others. and also mentioned in ticket that dev consensus is currently towards one commit per PR. so please keep track on that.

https://lists.apache.org/thread/f0tx0p7jd9f2nhzpgp75hxk267hrmnd9

[edk12564]

Ahh, I see. I might have misunderstood the direction at first. You're right, let's wait this one out.

[Aman-Mittal]

@edk12564 There is another case i found which can affect the ticket and needs discussion, which is signed commit check, while there can be trailers but only one person can sign that commit, if that is not signed properly this can affect either PR check or can cause loosing credit if signer was not careful

[edk12564]

@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!

[adamsaghy]

87 actionable tasks: 36 executed, 37 from cache, 14 up-to-date
Execution failed for task ':rat'.
> A failure occurred while executing org.nosphere.apache.rat.RatWork
   > Apache Rat audit failure - 1 unapproved license
     	See file:///home/runner/work/fineract/fineract/build/reports/rat/index.html

[edk12564]

87 actionable tasks: 36 executed, 37 from cache, 14 up-to-date
Execution failed for task ':rat'.
> A failure occurred while executing org.nosphere.apache.rat.RatWork
   > Apache Rat audit failure - 1 unapproved license
     	See file:///home/runner/work/fineract/fineract/build/reports/rat/index.html

Sorry about that. I'll fix this now!