Skip to content
Merged
Changes from all commits
Commits
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
54 changes: 54 additions & 0 deletions .github/workflows/sycl-check-ready-to-merge-prs.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# This workflow checks for ready-to-merge PRs - if a PR is open, not a draft,
# passed all checks, and has been approved, it will ping @intel/llvm-gatekeepers
# if this group has not already been mentioned or if the last mention was more
# than $days days ago.

name: Check ready-to-merge PRs

on:
schedule:
- cron: '0 * * * *' # every hour
Copy link
Contributor

Choose a reason for hiding this comment

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

Once a day seems more reasonable to me.
What is the reason to run this workflow every hour?

Copy link
Contributor

@sarnex sarnex Sep 2, 2025

Choose a reason for hiding this comment

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

the workflow runs once an hour but it only adds a comment if there has been no ping in 3 days

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bader the more often we launch this workflow, the faster gatekeepers get a notification about ready-to-merge PRs. But if you think it's too often, we can switch to a daily basis. Should we?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know. The PR description doesn't have problem statement. Are you trying to merge PRs faster? How fast is good enough?

Why not to make it immediate? You need to make all the checks you mentioned in the PR description at two events:

  • PR is approved.
  • Pre-commit testing is finished.

Another alternative: enable auto-merge for "ready-to-merge" PRs. Is there any reason for gatekeepers to not merge such PRs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another alternative: enable auto-merge for "ready-to-merge" PRs. Is there any reason for gatekeepers to not merge such PRs?

I also thought of this, we do this for the internal fork already. I couldn't find out how to do it in the GitHub settings but IMO this is the best long-time thing to do. We probably need to get a sign off from stewart though

Copy link
Contributor

Choose a reason for hiding this comment

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

The check verifying that author's e-mail is publicly visible doesn't work properly, so we have to have humans in the loop at least for that.

Copy link
Contributor

@bader bader Sep 3, 2025

Choose a reason for hiding this comment

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

FYI: PRs are approved by humans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand an auto-merge should be enabled for every PR manually, so it's not possible to auto-merge all ready-to-merge PRs - if that's what you're talking about.

Copy link
Contributor

Choose a reason for hiding this comment

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

What we have internally is a bit different, the PR isn't auto merged but all developers are able to merge the PR themselves if all requirements are met

Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand an auto-merge should be enabled for every PR manually

I'm 99.(9)% sure it can be scripted.

Merging by developers with write access to the repository sounds good to me too.

workflow_dispatch:

permissions: read-all

jobs:
notify-ready-prs:
permissions:
pull-requests: write
runs-on: ubuntu-latest
steps:
- name: Check
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
# Number of days before repeating the gatekeepers ping
days=3
days_in_seconds=$((days*24*60*60))

# Function to ping gatekeepers and print debug info
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the function doesn't print the debug info. Should it be removed from the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is some kind of debug info, isn't it?:D
echo "Pinged @intel/llvm-gatekeepers for https://github.com/intel/llvm/pull/$pr_number"

Copy link
Contributor

@dm-vodopyanov dm-vodopyanov Sep 1, 2025

Choose a reason for hiding this comment

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

Ah, if this is a debug info, then okay;) It looked like logging for me

ping_gatekeepers() {
pr_number=$1
gh pr comment "$pr_number" --repo intel/llvm --body "@intel/llvm-gatekeepers please consider merging"
echo "Pinged @intel/llvm-gatekeepers for https://github.com/intel/llvm/pull/$pr_number"
}

# Get the list of suitable PRs
prs=$(gh pr list --search "is:open review:approved draft:no status:success" --repo intel/llvm --json number --jq '.[].number')
now=$(date -u +%s)
for pr in $prs; do
# Get the timestamp of the latest comment mentioning @intel/llvm-gatekeepers
latest_ts=$(gh pr view $pr --repo intel/llvm --json comments \
--jq '[.comments[] | select(.body | test("@intel/llvm-gatekeepers")) | .createdAt] | last')
# If there is no previous mention, ping the gatekeepers
if [[ -z "$latest_ts" ]]; then
ping_gatekeepers "$pr"
# If the latest mention is older than $days, ping the gatekeepers again
else
comment_time=$(date -u -d "$latest_ts" +%s)
age=$((now - comment_time))
if (( age >= days_in_seconds )); then
ping_gatekeepers "$pr"
fi
fi
done
Loading