-
Notifications
You must be signed in to change notification settings - Fork 106
ci: update merge conflict bot to trigger on main #970
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
base: main
Are you sure you want to change the base?
Changes from 18 commits
1158375
353fc4f
cde4093
dfdca52
8fe94de
5b5b068
5ac5065
1388019
c8fcec0
69f4371
383e5ed
d01a4d9
f5f41f1
2b6bdd5
2b07a7e
4d23480
f58be13
eb39ae1
56948cd
af8b9cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| // scripts/merge_conflict_helpers.js | ||
|
|
||
| const BOT_SIGNATURE = ''; | ||
|
|
||
| module.exports = async ({ github, context, core }) => { | ||
| const { owner, repo } = context.repo; | ||
|
|
||
| // fetch PR with retry logic for unknown state | ||
| async function getPrWithRetry(prNumber) { | ||
| for (let i = 0; i < 10; i++) { | ||
| const { data: pr } = await github.rest.pulls.get({ | ||
| owner, repo, pull_number: prNumber | ||
| }); | ||
|
|
||
| if (pr.mergeable_state !== 'unknown') return pr; | ||
|
|
||
| console.log(`PR #${prNumber} state is 'unknown'. Retrying (${i+1}/10)...`); | ||
| await new Promise(r => setTimeout(r, 2000)); | ||
| } | ||
| const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: prNumber }); | ||
| return pr; | ||
| } | ||
|
|
||
| // post comment | ||
| async function notifyUser(prNumber) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://docs.github.com/en/rest/issues/comments?apiVersion=2022-11-28#list-issue-comments i think that this will check only 30 comments with default pagination, this could be a limit @exploreriii, i think that the default value is 30 also for the PRs, so if we have 34 open PRs it could be a problem. It's a thought and it's debatable for sure :D
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep we can definitely paginate if its easy, but yes, currently i think we can get away without |
||
| const { data: comments } = await github.rest.issues.listComments({ | ||
| owner, repo, issue_number: prNumber, | ||
| }); | ||
|
|
||
| if (comments.some(c => c.body.includes(BOT_SIGNATURE))) { | ||
| console.log(`Already commented on PR #${prNumber}. Skipping.`); | ||
| return; | ||
| } | ||
|
|
||
| const body = `Hi, this is MergeConflictBot.\nYour pull request cannot be merged because it contains **merge conflicts**.\n\nPlease resolve these conflicts locally and push the changes.\n\nTo assist you, please read:\n- [Resolving Merge Conflicts](docs/sdk_developers/merge_conflicts.md)\n- [Rebasing Guide](docs/sdk_developers/rebasing.md)\n\nThank you for contributing!\nFrom the Hiero Python SDK Team\n\n${BOT_SIGNATURE}`; | ||
|
|
||
| await github.rest.issues.createComment({ | ||
| owner, repo, issue_number: prNumber, body: body | ||
| }); | ||
| } | ||
|
|
||
| //set commit status | ||
| async function setCommitStatus(sha, state, description) { | ||
| await github.rest.repos.createCommitStatus({ | ||
| owner, repo, sha: sha, state: state, | ||
| context: 'Merge Conflict Detector', | ||
| description: description, | ||
| target_url: `${process.env.GITHUB_SERVER_URL}/${owner}/${repo}/actions/runs/${context.runId}` | ||
| }); | ||
| } | ||
|
|
||
| //main | ||
| let prsToCheck = []; | ||
|
|
||
| //push to main | ||
| if (context.eventName === 'push') { | ||
| console.log("Triggered by Push to Main. Fetching all open PRs..."); | ||
| const { data: openPrs } = await github.rest.pulls.list({ | ||
| owner, repo, state: 'open', base: 'main' | ||
| }); | ||
| prsToCheck = openPrs.map(pr => pr.number); | ||
| } | ||
| //PR update | ||
| else { | ||
| console.log("Triggered by PR update."); | ||
| prsToCheck.push(context.payload.pull_request.number); | ||
| } | ||
|
|
||
| let hasFailure = false; | ||
|
|
||
| for (const prNumber of prsToCheck) { | ||
| console.log(`Checking PR #${prNumber}...`); | ||
| const pr = await getPrWithRetry(prNumber); | ||
|
|
||
| if (pr.mergeable_state === 'dirty') { | ||
| console.log(`Conflict detected in PR #${prNumber}`); | ||
| await notifyUser(prNumber); | ||
|
|
||
| if (context.eventName === 'push') { | ||
| await setCommitStatus(pr.head.sha, 'failure', 'Conflicts detected with main'); | ||
| } else { | ||
| core.setFailed(`Merge conflicts detected in PR #${prNumber}.`); | ||
| hasFailure = true; | ||
| } | ||
| } else { | ||
| console.log(`PR #${prNumber} is clean.`); | ||
| if (context.eventName === 'push') { | ||
| await setCommitStatus(pr.head.sha, 'success', 'No conflicts detected'); | ||
| } | ||
| } | ||
| } | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,71 +1,41 @@ | ||
| name: PythonBot - Check Merge Conflicts | ||
| name: Merge Conflict Bot | ||
|
|
||
| on: | ||
| pull_request_target: | ||
| types: [opened, synchronize, reopened] | ||
| push: | ||
| branches: | ||
| - main | ||
|
|
||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| issues: write | ||
| statuses: write | ||
|
|
||
| concurrency: | ||
| group: "check-conflicts-${{ github.event.pull_request.number }}" | ||
| group: "check-conflicts-${{ github.event.pull_request.number || github.sha }}" | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| check-conflicts: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
cheese-cakee marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| - name: Harden the runner (Audit all outbound calls) | ||
| uses: step-security/harden-runner@20cf305ff2072d973412fa9b1e3a4f227bda3c76 # v2.14.0 | ||
| uses: step-security/harden-runner@20cf305ff2072d973412fa9b1e3a4f227bda3c76 | ||
| with: | ||
| egress-policy: audit | ||
|
|
||
| - name: Check for merge conflicts | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| PR_NUMBER=${{ github.event.pull_request.number }} | ||
| REPO="${{ github.repository }}" | ||
| echo "Checking merge status for PR #$PR_NUMBER in repository $REPO..." | ||
| for i in {1..10}; do | ||
| PR_JSON=$(gh api repos/$REPO/pulls/$PR_NUMBER) | ||
| MERGEABLE_STATE=$(echo "$PR_JSON" | jq -r '.mergeable_state') | ||
| echo "Attempt $i: Current mergeable state: $MERGEABLE_STATE" | ||
| if [ "$MERGEABLE_STATE" != "unknown" ]; then | ||
| break | ||
| fi | ||
| echo "State is 'unknown', waiting 2 seconds..." | ||
| sleep 2 | ||
| done | ||
|
|
||
| if [ "$MERGEABLE_STATE" = "dirty" ]; then | ||
| COMMENT=$(cat <<EOF | ||
| Hi, this is MergeConflictBot. | ||
| Your pull request cannot be merged because it contains **merge conflicts**. | ||
| Please resolve these conflicts locally and push the changes. | ||
| To assist you, please read: | ||
| - [Resolving Merge Conflicts](docs/sdk_developers/merge_conflicts.md) | ||
| - [Rebasing Guide](docs/sdk_developers/rebasing.md) | ||
| Thank you for contributing! | ||
| From the Hiero Python SDK Team | ||
| EOF | ||
| ) | ||
| gh pr view $PR_NUMBER --repo $REPO --json comments --jq '.comments[].body' | grep -F "MergeConflictBot" >/dev/null || \ | ||
| (gh pr comment $PR_NUMBER --repo $REPO --body "$COMMENT" && echo "Comment added to PR #$PR_NUMBER") | ||
| - name: Check for merge conflicts | ||
| uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd | ||
| with: | ||
| script: | | ||
| const path = require('path') | ||
| const scriptPath = path.join(process.env.GITHUB_WORKSPACE, '.github/scripts/merge_conflict_helpers.js') | ||
| exit 1 | ||
| else | ||
| echo "No merge conflicts detected (State: $MERGEABLE_STATE)." | ||
| fi | ||
| console.log(`Loading script from: ${scriptPath}`) | ||
| const script = require(scriptPath) | ||
| await script({github, context, core}) | ||
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.
if we use this as signature what will happen if a user (not a bot or not that specific bot) has already commented on the pr? will it detect that comment having the BOT_SIGNATURE ?
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 have added a unique HTML comment signature so the bot won't confuse user comments for its own @AntonioCeppellini
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 think the
BOT_SIGNATUREis an empty string now?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.
@cheese-cakee i still see an empty string like @MonaaEid
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.
you could use something simple like
[merge-conflict bot]like we did in https://github.com/hiero-ledger/hiero-sdk-python/blob/main/.github/workflows/bot-verified-commits.yml#L50There 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.
thanks for the example! I've switched the signature to [merge-conflict bot] as you suggested.
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.
why using bot signature? If there is another merge conflict the bot will not be able to warn the author again.
can someone explain this please
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.
nice @cheese-cakee :D, have you also tested the case that a pr already has comments in it from your fork?
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.
@MonaaEid The signature is necessary to prevent spam. Since this workflow runs on every push, without this check, the bot would post a new comment every single time a user pushes code while a conflict exists. This ensures it alerts the user once.
@AntonioCeppellini Yes tested! Since the signature [merge-conflict bot] is unique I verified that the bot correctly ignores all standard user comments and only identifies its own previous warnings.