Skip to content

Use reusable workflow for pull request merging using GitHub App auth - #1243

Merged
sachintu47 merged 11 commits into
mainfrom
bump_merge_workflow
Aug 26, 2026
Merged

Use reusable workflow for pull request merging using GitHub App auth#1243
sachintu47 merged 11 commits into
mainfrom
bump_merge_workflow

Conversation

@sachintu47

@sachintu47 sachintu47 commented Aug 26, 2026

Copy link
Copy Markdown
Member

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Content Update

Category

  • zopen build framework
  • zopen package manager
  • Documentation
  • CI/CD
  • Tools

Description

Related Issues

  • Related Issue #
  • Closes #

[optional] Are there any post-deployment tasks or follow-up actions required?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a reusable GitHub Actions workflow to auto-merge certain pull requests using GitHub App authentication, and updates the existing build/test workflow to invoke that reusable workflow instead of embedding merge logic inline.

Changes:

  • Added a reusable workflow (merge_pr.yml) that generates a GitHub App token and merges a PR (optionally deleting the source branch).
  • Replaced the inline “bump PR auto-merge” GitHub Script block in build_and_test.yml with a call to the reusable workflow.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
.github/workflows/merge_pr.yml New reusable workflow that creates an App token, merges the PR, and optionally deletes the branch.
.github/workflows/build_and_test.yml Switches from inline merge logic to calling the reusable merge workflow for bump branches.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/merge_pr.yml
Comment thread .github/workflows/build_and_test.yml Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Sachin <32639496+sachintu47@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 06:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/merge_pr.yml:30

  • continue-on-error: true will mark this job successful even when the merge/delete-ref API calls fail, which makes auto-merge failures hard to notice and can silently leave bump PRs unmerged. Also, actions/github-script@v6 runs on a deprecated Node.js runtime; prefer the current major.
      - name: Merge Pull Request 
        continue-on-error: true
        uses: actions/github-script@v6

.github/workflows/merge_pr.yml:38

  • The auto-merge script now merges any non-fork PR without verifying it’s actually a bump-action PR. Previously the merge was gated on the last commit’s committer email and the branch prefix; dropping that check makes it possible for any PR from a bump* branch to be auto-merged once this workflow is invoked. Reintroduce the committer-email/branch guard before calling pulls.merge to preserve the prior safety behavior.
            const pr = context.payload.pull_request;
            if (!pr || pr.head.repo.fork) return;
            
            const { owner, repo } = context.repo;
            const prNumber = pr.number;

Signed-off-by: Sachin <32639496+sachintu47@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 07:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/merge_pr.yml:30

  • In this reusable workflow, continue-on-error: true will mark the merge step as successful even when the merge API call fails (e.g., conflicts, branch protection, missing permissions). That can silently leave PRs unmerged while the workflow appears green. Prefer letting the step fail, or explicitly catching and logging errors while still failing the job when merge is expected to succeed.
      - name: Merge Pull Request 
        continue-on-error: true
        uses: actions/github-script@v6

Comment thread .github/workflows/build_and_test.yml
Comment thread .github/workflows/build_and_test.yml Outdated
…flow at job level

Co-authored-by: sachintu47 <32639496+sachintu47@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 07:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

.github/workflows/merge_pr.yml:32

  • If the app-token step is skipped (e.g., fork PRs / non-PR events), this step will still run and reference steps.app-token.outputs.token, causing a runtime failure. Gate the merge step with the same non-fork PR condition (and remove the trailing whitespace in the step name while here).
      - name: Merge Pull Request 
        continue-on-error: true
        uses: actions/github-script@v6
        with:
          github-token: ${{ steps.app-token.outputs.token }}

.github/workflows/build_and_test.yml:187

  • This reusable workflow references secrets.MERGE_APP_ID / secrets.MERGE_APP_PRIVATE_KEY when calling merge_pr.yml, but build_and_test.yml (as a workflow_call reusable workflow) currently only declares Jenkins secrets. Callers won’t be able to pass these merge secrets unless they’re added to on.workflow_call.secrets here (ideally optional) and the merge job is gated on them being present.
    uses: ./.github/workflows/merge_pr.yml
    with:
      delete_branch: true
    secrets:
      MERGE_APP_ID: ${{ secrets.MERGE_APP_ID }}
      MERGE_APP_PRIVATE_KEY: ${{ secrets.MERGE_APP_PRIVATE_KEY }}

Comment thread .github/workflows/merge_pr.yml
Comment thread .github/workflows/build_and_test.yml
…ter email

Co-authored-by: sachintu47 <32639496+sachintu47@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 07:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread .github/workflows/build_and_test.yml
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Sachin <32639496+sachintu47@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 07:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

.github/workflows/build_and_test.yml:156

  • The step output key is named is-bump (contains a hyphen), but it’s referenced using dot notation (steps.check.outputs.is-bump). In GitHub Actions expressions, keys with - must be accessed with bracket notation, otherwise this can be parsed as subtraction and the job output won’t be set correctly.

This issue also appears on line 181 of the same file.

    outputs:
      is-bump: ${{ steps.check.outputs.is-bump }}

.github/workflows/merge_pr.yml:47

  • The merge API call does not pass the expected head SHA. Without sha, the PR could be merged even if new commits are pushed between validation and merge, which is risky for automation. Pass sha: pr.head.sha so GitHub will reject the merge if the head changed.
              owner,
              repo,
              pull_number: prNumber,
              merge_method: "merge"
            });

.github/workflows/build_and_test.yml:185

  • pulls.listCommits returns only the first 30 commits by default, so commits.at(-1) is not guaranteed to be the PR’s actual HEAD commit on larger PRs. This can misclassify is-bump. Prefer reading the commit metadata for pr.head.sha directly.
            const { data: commits } = await github.rest.pulls.listCommits({
              owner: context.repo.owner,
              repo: context.repo.repo,
              pull_number: pr.number
            });

Signed-off-by: Sachin <32639496+sachintu47@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 09:08
Copilot AI review requested due to automatic review settings August 26, 2026 12:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

.github/workflows/build_and_test.yml:193

  • The auto-merge gate relies on the last commit’s commit.committer.email matching bump-action@github, but git commit emails are user-controlled. Any user who can push a branch in this repo could spoof that email on a bump-* branch and get their PR auto-merged via the GitHub App token. If this is meant to merge only trusted automation PRs, add a stronger signal (e.g., require a label set by a trusted workflow, or validate the PR author/login/app).
            const email = commits.at(-1).commit.committer.email;
            if (email !== "bump-action@github") {
              console.log(`⏭️  Skipping merge: PR #${pr.number} not created by bump automation (committer: ${email})`);
              core.setOutput('is-bump', 'false');
            } else {
              core.setOutput('is-bump', 'true');
            }

Comment thread .github/workflows/merge_pr.yml Outdated
Comment thread .github/workflows/merge_pr.yml
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Sachin <32639496+sachintu47@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 13:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

.github/workflows/build_and_test.yml:191

  • The bump-PR gate is still based on branch.startsWith('bump-') plus the last commit’s committer email. Both are user-controlled for anyone with push rights, so an internal contributor could spoof bump-action@github on a bump-* branch and get auto-merged. Consider additionally validating the PR author/sender is the expected automation (e.g., pr.user.type === 'Bot' and an allowlisted pr.user.login/github.actor), or validating that the PR was created by a specific GitHub App/workflow identity.
            const email = commits.at(-1).commit.committer.email;
            if (email !== "bump-action@github") {
              console.log(`⏭️  Skipping merge: PR #${pr.number} not created by bump automation (committer: ${email})`);
              core.setOutput('is-bump', 'false');
            } else {

Comment thread .github/workflows/build_and_test.yml
… in merge_pr.yml

Co-authored-by: sachintu47 <32639496+sachintu47@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 26, 2026 13:43
Co-authored-by: sachintu47 <32639496+sachintu47@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/merge_pr.yml:4

  • This workflow does not set explicit permissions, while other workflows in this repo do (e.g., .github/workflows/jenkins-test.yml:3-6, .github/workflows/labeler.yml:6-9). Since this workflow uses a GitHub App token and does not need GITHUB_TOKEN permissions, consider explicitly disabling default token permissions to reduce blast radius if future steps are added.
name: "MergePR"

on:
  workflow_call:

.github/workflows/merge_pr.yml:37

  • continue-on-error: true will cause merge failures (e.g., conflicts, branch protection, missing GitHub App permissions) to be ignored and the workflow to succeed, making automation failures hard to detect. This also reintroduces the behavior previously removed in review feedback.
      - name: Merge Pull Request 
        uses: actions/github-script@v6
        with:

.github/workflows/build_and_test.yml:203

  • MERGE_APP_ID / MERGE_APP_PRIVATE_KEY are referenced here, but this reusable workflow (build_and_test.yml) does not declare them under on.workflow_call.secrets (it currently only declares JENKINS_USER / JENKINS_API_TOKEN). In reusable workflows, undeclared secrets typically won’t be passed through from the caller, so these may resolve empty and cause the merge workflow to fail even when the caller has the secrets.
    with:
      delete_branch: true
    secrets:
      MERGE_APP_ID: ${{ secrets.MERGE_APP_ID }}
      MERGE_APP_PRIVATE_KEY: ${{ secrets.MERGE_APP_PRIVATE_KEY }}

Copilot AI review requested due to automatic review settings August 26, 2026 13:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

.github/workflows/build_and_test.yml:167

  • context.payload.pull_request is assumed to exist, but if this job ever runs without a PR payload (e.g., event changes or manual re-runs in some contexts), pr.head... will throw and the job output is-bump will never be set. Add a defensive !pr guard that sets is-bump=false before accessing pr.head.
            const pr = context.payload.pull_request;

            // Skip cross-repository fork PRs
            if (pr.head.repo.full_name !== pr.base.repo.full_name) {

.github/workflows/merge_pr.yml:44

  • The merge workflow silently returns when there is no PR in the event payload or when the PR is cross-repo (fork). That makes it hard to diagnose why a merge didn’t happen when this workflow is reused in different callers. Add explicit log messages before returning so the workflow run clearly explains the skip reason.
            const pr = context.payload.pull_request;
            if (!pr) return;

            // Skip cross-repository PRs (e.g., forks)
            if (pr.head.repo.full_name !== pr.base.repo.full_name) return;

@IgorTodorovskiIBM IgorTodorovskiIBM left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@sachintu47
sachintu47 merged commit 0dc1d25 into main Aug 26, 2026
2 of 3 checks passed
@sachintu47
sachintu47 deleted the bump_merge_workflow branch August 26, 2026 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants