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

[edk12564]

Description

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.

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:

Success path + 2nd run failure due to 2 commits from same user:

Error message on more than one commit per user, with git email properly configured is taking a while. One more picture incoming!

Note: there were a few wording changes between pictures.

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?

[Aman-Mittal]

@meonkeys Your input is valued here i will give my input.

for point 1 I think, It doesn;t raise security concern, as we are not executing any external code (eg any suspicious JS file or third party dependency it will be based on implementation)

for point 2, i think we can simply add check that PRs that do not have stale tag can be informed. As per stale information inactive PRs are automatically Closed by stale bot. And to reduce spaming further we can make it comment one time and let it edit the existing comment.

[edk12564]

I also just realized I must have reverted to a previous version while switching branches. I have an old variable I used that I thought I removed already. Repushing now!!