-
Notifications
You must be signed in to change notification settings - Fork 2.3k
FINERACT-2436: 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.
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
apache:develop
Choose a base branch
from
edk12564:FINERACT-2462
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+57
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| name: Fineract PR One Commit Per User Check | ||
|
|
||
|
|
||
| on: | ||
| 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." | ||
| 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 }} | ||
| 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.' | ||
| 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 | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.