-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Add merge workflow #22120
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: master
Are you sure you want to change the base?
Add merge workflow #22120
Changes from all commits
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,141 @@ | ||
| <?php | ||
|
|
||
| function try_run(array $args): bool { | ||
| $command = implode(' ', array_map('escapeshellarg', $args)); | ||
| echo "> $command\n"; | ||
| passthru($command, $status); | ||
| return $status === 0; | ||
| } | ||
|
|
||
| function run(array $args, ?string $failure_message = null) { | ||
| if (!try_run($args)) { | ||
| throw new RuntimeException($failure_message ?? 'Command failed'); | ||
| } | ||
| } | ||
|
|
||
| function origin_branch_exists(string $branch): bool { | ||
| return try_run(['git', 'show-ref', '--verify', '--quiet', "refs/remotes/origin/$branch"]); | ||
| } | ||
|
|
||
| function find_next_release_branch(string $current): ?string { | ||
| if ($current === 'master') { | ||
| return null; | ||
| } | ||
|
|
||
| if (!preg_match('(^PHP-(?<major>\d+)\.(?<minor>\d+)$)', $current, $matches)) { | ||
| return null; | ||
| } | ||
|
|
||
| $major = $matches['major']; | ||
| $minor = $matches['minor']; | ||
|
|
||
| $next = "PHP-$major." . ($minor + 1); | ||
| if (origin_branch_exists($next)) { | ||
| return $next; | ||
| } | ||
|
|
||
| $next = 'PHP-' . ($major + 1) . '.0'; | ||
| if (origin_branch_exists($next)) { | ||
| return $next; | ||
| } | ||
|
|
||
| return 'master'; | ||
| } | ||
|
|
||
| function find_release_branches(string $target): array { | ||
| $branches = [$target]; | ||
| while (null !== $next = find_next_release_branch(end($branches))) { | ||
| $branches[] = $next; | ||
| } | ||
| return $branches; | ||
| } | ||
|
|
||
| function merge_pr_into_target(string $pr_sha, string $pr_first_sha, string $target, string $message, string $description): string { | ||
| $author = trim((string) shell_exec('git log -1 --format=' . escapeshellarg('%an <%ae>') . ' ' . escapeshellarg($pr_first_sha))); | ||
|
|
||
| run(['git', 'checkout', '-B', $target, "refs/remotes/origin/$target"]); | ||
| run(['git', 'merge', '--squash', $pr_sha], | ||
| failure_message: "Failed to squash PR into $target."); | ||
| run(['git', 'commit', "--author=$author", '-m', $message, '-m', wrap_commit_message($description)]); | ||
| $squashed_sha = trim((string) shell_exec('git rev-parse HEAD')); | ||
|
|
||
| return $squashed_sha; | ||
| } | ||
|
|
||
| function merge_upwards(array $branches) { | ||
| for ($i = 1; $i < count($branches); $i++) { | ||
| $prev = $branches[$i - 1]; | ||
| $current = $branches[$i]; | ||
| run(['git', 'checkout', '-B', $current, "refs/remotes/origin/$current"]); | ||
| run(['git', 'merge', '--no-ff', '--no-edit', $prev], | ||
| failure_message: "Failed to merge $prev into $current."); | ||
| } | ||
| } | ||
|
|
||
| function push_pr_branch(string $url, string $branch, string $squashed_sha, string $original_sha) { | ||
| run(['git', 'push', "--force-with-lease=$branch:$original_sha", $url, "$squashed_sha:refs/heads/$branch"], | ||
|
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. This might fail if the PR author forgets to tick the box to allow maintainer commits.
Member
Author
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. Indeed. This step is technically optional, but I'm doing it so that the PR is marked as "merged". I suppose we can close it manually if this step fails, but I also don't think many people tick off this box. But I'm happy to add this fallback if you like.
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. Yes, handling the fallback would be better.
Member
Author
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. We'll need to differentiate between a |
||
| failure_message: 'Failed to push rebased PR branch.'); | ||
| } | ||
|
|
||
| function push_release_branches(array $branches) { | ||
| run(['git', 'push', '--atomic', 'origin', ...$branches], | ||
| failure_message: 'Failed to push release branches.'); | ||
| } | ||
|
|
||
| function wrap_commit_message(string $message, int $width = 80): string { | ||
| $lines = explode("\n", $message); | ||
| $result = []; | ||
| $code_section = false; | ||
|
|
||
| foreach ($lines as $line) { | ||
| if (preg_match('(^\s*```)', $line)) { | ||
| $code_section = !$code_section; | ||
| $result[] = $line; | ||
| continue; | ||
| } | ||
|
|
||
| if ($code_section) { | ||
| $result[] = $line; | ||
| continue; | ||
| } | ||
|
|
||
| if ($line === '' || preg_match('(^\s)', $line)) { | ||
| $result[] = $line; | ||
| continue; | ||
| } | ||
|
|
||
| $result[] = wordwrap($line, $width, "\n", false); | ||
| } | ||
|
|
||
| return implode("\n", $result); | ||
| } | ||
|
|
||
| function main(): int { | ||
| $target = getenv('TARGET_BRANCH'); | ||
| $pr_number = getenv('PR_NUMBER'); | ||
| $pr_first_sha = getenv('PR_FIRST_SHA'); | ||
| $pr_sha = getenv('PR_SHA'); | ||
| $pr_ref = getenv('PR_REF'); | ||
| $pr_repo_url = getenv('PR_REPO_URL'); | ||
| $pr_title = getenv('PR_TITLE'); | ||
| $pr_description = getenv('PR_DESCRIPTION'); | ||
|
|
||
| $release_branches = find_release_branches($target); | ||
|
|
||
| try { | ||
| $squashed_sha = merge_pr_into_target($pr_sha, $pr_first_sha, $target, "$pr_title (GH-$pr_number)", $pr_description); | ||
| merge_upwards($release_branches); | ||
| push_pr_branch($pr_repo_url, $pr_ref, $squashed_sha, $pr_sha); | ||
| push_release_branches($release_branches); | ||
|
Comment on lines
+128
to
+129
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. This is not atomic as what if pr_branch gets pushed and push_release_branches fails, this can happen when github is having issues which is a lot lately.
Member
Author
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. Indeed, but sadly I didn't find a way to push both the PR and release branches atomically, given that the PR branch comes from a different origin. I thought this was acceptable, given that we sometimes do this manually as well. The only solution I can think of is to revert the PR by pushing the previous commit, but given this might also fail, that would also be an incomplete solution. Do you have another suggestion?
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. I think adding error handling and a PR comment in case this goes wrong might be a good compromise. |
||
| } catch (Throwable $e) { | ||
| if (false !== ($github_output = getenv('GITHUB_OUTPUT'))) { | ||
| file_put_contents($github_output, "fail_reason<<EOF\n{$e->getMessage()}\nEOF\n", FILE_APPEND); | ||
| } | ||
| fwrite(STDERR, "::error::{$e->getMessage()}\n"); | ||
| return 1; | ||
| } | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| exit(main()); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| name: Merge PR | ||
|
|
||
| on: | ||
| pull_request_target: | ||
| types: [labeled] | ||
|
Comment on lines
+4
to
+5
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. Using pull_request_target should be avoided. I would suggest using workflow_dispatch that gets triggered by a separate small workflow that runs on |
||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
|
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. Add concurrency, this should not run concurrently. |
||
| jobs: | ||
| merge_pr: | ||
| name: Merge PR | ||
| if: github.event.label.name == 'Merge' | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: write | ||
| pull-requests: write | ||
| steps: | ||
| - name: git checkout | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 0 | ||
| filter: blob:none | ||
| ref: ${{ github.event.pull_request.base.ref }} | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
|
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. Please check that merging with GITHUB_TOKEN triggers CI for merged branches, This might need an app token. |
||
|
|
||
| - name: git config | ||
| run: | | ||
| git config user.name "PHP GH Bot" | ||
| git config user.email "gh-bot@php.net" | ||
| git config merge.NEWS.name "Keep the NEWS file" | ||
| git config merge.NEWS.driver "touch %A" | ||
| git config merge.log true | ||
|
|
||
| - name: Fetch PR head | ||
| env: | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| run: | | ||
| git fetch origin "refs/pull/${PR_NUMBER}/head" | ||
|
|
||
| - name: Merge PR | ||
| id: merge | ||
| env: | ||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||
| PR_FIRST_SHA: $(git log --reverse --format=%H ${{ github.event.pull_request.base.sha }}..${{ github.event.pull_request.head.sha }} | head -n1) | ||
|
Member
Author
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. This doesn't actually work ofc and will need to |
||
| PR_SHA: ${{ github.event.pull_request.head.sha }} | ||
| PR_REF: ${{ github.event.pull_request.head.ref }} | ||
|
Comment on lines
+45
to
+47
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. There should be a check to stop this workflow before pushing changes if the PR_SHA does not match the PR head as it changed between the label event and the workflow merging and pushing. This can happen when the nightly workflow is running and other workflows runs are delayed. |
||
| PR_REPO_URL: ${{ github.event.pull_request.head.repo.clone_url }} | ||
| PR_TITLE: ${{ github.event.pull_request.title }} | ||
| PR_DESCRIPTION: ${{ github.event.pull_request.body }} | ||
| TARGET_BRANCH: ${{ github.event.pull_request.base.ref }} | ||
| run: | | ||
| # Use merge script from master to avoid syncing to lower branches. | ||
| git show origin/master:.github/scripts/merge_pr.php > "$RUNNER_TEMP/merge_pr.php" | ||
| php "$RUNNER_TEMP/merge_pr.php" | ||
|
|
||
| - name: Report failure | ||
| if: failure() | ||
| uses: actions/github-script@v9 | ||
| env: | ||
| FAIL_REASON: ${{ steps.merge.outputs.fail_reason }} | ||
| with: | ||
| script: | | ||
| const reason = process.env.FAIL_REASON || 'Unknown error.'; | ||
| const runUrl = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`; | ||
|
|
||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| body: `Merge failed: ${reason}\n\n[View workflow run](${runUrl}).`, | ||
| }); | ||
|
|
||
| - name: Remove Merge label | ||
| if: ${{ always() }} | ||
| uses: actions/github-script@v9 | ||
| with: | ||
| script: | | ||
| try { | ||
| await github.rest.issues.removeLabel({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| name: 'Merge', | ||
| }); | ||
| } catch (e) { | ||
| core.warning(`Could not remove the 'Merge' label: ${e.message}`); | ||
| } | ||
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.
This is great, but what if someone labels an old open PR targeting an unsupported branch by mistake and the automation goes off merging it. I would suggest we keep a manifest of branches that this goes through and that is maintained when we branch off to avoid any unexpected merges.
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 believe that should be rejected due to our push rules. But I'm happy to test.