diff --git a/.asf.yaml b/.asf.yaml index e4652b3f86ec..64a0985ea149 100644 --- a/.asf.yaml +++ b/.asf.yaml @@ -27,9 +27,27 @@ github: projects: true discussions: true + # Pull requests are merged with the squash button. The commit + # message is the pull request title and description. + enabled_merge_buttons: + squash: true + squash_commit_message: PR_TITLE_AND_DESC + merge: false + rebase: false + protected_branches: # protect main against force push - main: {} + main: + required_status_checks: + # Don't require branches to be up to date before merging. + strict: false + # The "Merge check" job in + # .github/workflows/dev_pr_merge_check.yml. The merge button + # is disabled while the pull request title isn't + # "GH-XXX: [Component] Title" or the issue isn't open and + # assigned. + contexts: + - Merge check collaborators: - alinaliBQ diff --git a/.github/workflows/dev_pr/merge_check.js b/.github/workflows/dev_pr/merge_check.js new file mode 100644 index 000000000000..e46783381528 --- /dev/null +++ b/.github/workflows/dev_pr/merge_check.js @@ -0,0 +1,83 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + + +const helpers = require("./helpers.js"); + +// GH-XXX: [Component] Title or MINOR: [Component] Title +const TITLE_PATTERN = /^(?:GH-(\d+)|MINOR): (?:\[[^\]]+\])+ \S/; + +/** + * Checks whether the pull request can be merged with the merge + * button: + * - The title is "GH-XXX: [Component] Title" or + * "MINOR: [Component] Title". It's used as the commit title. + * - The issue in the title exists and is open. It's closed by + * merged.js on merge. + * - The issue is assigned to someone. issue_check.js assigns it to + * the pull request author if possible. + * + * @param {Object} github + * @param {Object} context + * @returns {String[]} The found problems. + */ +async function check(github, context) { + const title = context.payload.pull_request.title; + const matched = TITLE_PATTERN.exec(title); + if (!matched) { + return [ + `The title must be "GH-XXX: [Component] Title" or ` + + `"MINOR: [Component] Title": ${title}`, + ]; + } + const issueID = matched[1]; + if (!issueID) { + return []; + } + + const issueInfo = await helpers.getGitHubInfo( + github, context, issueID, context.payload.number); + if (!issueInfo) { + return [`GitHub issue #${issueID} could not be retrieved.`]; + } + if (issueInfo.pull_request) { + return [`#${issueID} is a pull request, not an issue.`]; + } + const errors = []; + if (issueInfo.state !== "open") { + errors.push(`GitHub issue #${issueID} is already closed.`); + } + if (!issueInfo.assignees.length) { + errors.push(`GitHub issue #${issueID} is not assigned to anyone.`); + } + return errors; +} + +module.exports = async ({github, context, core}) => { + // The "Dev PR" workflow assigns the issue at the same time. Wait + // for it. + let errors = await check(github, context); + for (let i = 0; i < 6 && errors.length > 0 && + TITLE_PATTERN.test(context.payload.pull_request.title); i++) { + await new Promise((resolve) => setTimeout(resolve, 10 * 1000)); + errors = await check(github, context); + } + if (errors.length > 0) { + core.setFailed(errors.join("\n")); + } +}; +module.exports.check = check; diff --git a/.github/workflows/dev_pr/merged.js b/.github/workflows/dev_pr/merged.js new file mode 100644 index 000000000000..12fdb448b6c2 --- /dev/null +++ b/.github/workflows/dev_pr/merged.js @@ -0,0 +1,216 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +const helpers = require("./helpers.js"); + +const VERSION_PATTERN = /^\d+\.\d+\.\d+$/; + +/** + * Compares two versions like "23.0.0" numerically. + * + * @param {String} a + * @param {String} b + * @returns {Number} negative if a < b, positive if a > b, 0 otherwise. + */ +function compareVersions(a, b) { + const aParts = a.split(".").map(Number); + const bParts = b.split(".").map(Number); + for (let i = 0; i < aParts.length; i++) { + if (aParts[i] !== bParts[i]) { + return aParts[i] - bParts[i]; + } + } + return 0; +} + +/** + * Chooses the version a pull request merged into main is released in. + * This mirrors get_candidate_fix_version() in dev/merge_arrow_pr.py. + * + * @param {Array} versions - The titles of the open milestones. + * @param {Array} maintenanceBranches - The maint-X.Y.Z branches. + * @returns {String} the version or null if no version can be chosen. + */ +function detectFixVersion(versions, maintenanceBranches) { + versions = versions.filter((version) => VERSION_PATTERN.test(version)); + const majorVersions = versions.filter((version) => version.endsWith(".0.0")); + if (versions.length > majorVersions.length) { + // If there is a future major release, suggest that + versions = majorVersions; + } + versions = versions + .filter((version) => !maintenanceBranches.includes(`maint-${version}`)) + .sort(compareVersions); + return versions.length > 0 ? versions[0] : null; +} + +/** + * Removes the PR workflow state labels from the merged pull request. + * + * @param {Object} github + * @param {Object} context + * @param {Object} pullRequest + */ +async function clearStateLabels(github, context, pullRequest) { + for (const label of pullRequest.labels) { + // All PR workflow state labels starts with "awaiting" + if (!label.name.startsWith("awaiting")) { + continue; + } + try { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: pullRequest.number, + name: label.name + }); + } catch (error) { + // The label may be already removed. + if (error.status !== 404) { + throw error; + } + } + } +} + +/** + * Assigns the milestone of the release the pull request is part of to the + * GitHub Issue. It's the milestone of the pull request if it has one. + * Otherwise an already assigned milestone is kept or the next major + * release is used. + * + * @param {Object} github + * @param {Object} context + * @param {Object} pullRequest + * @param {Object} issueInfo + */ +async function assignMilestone(github, context, pullRequest, issueInfo) { + if (pullRequest.milestone) { + if (issueInfo.milestone?.number !== pullRequest.milestone.number) { + await github.rest.issues.update({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: issueInfo.number, + milestone: pullRequest.milestone.number + }); + } + return; + } + if (issueInfo.milestone) { + return; + } + const milestones = await github.paginate(github.rest.issues.listMilestones, { + owner: context.repo.owner, + repo: context.repo.repo, + state: "open", + per_page: 100 + }); + const branches = await github.paginate(github.rest.repos.listBranches, { + owner: context.repo.owner, + repo: context.repo.repo, + per_page: 100 + }); + const fixVersion = detectFixVersion( + milestones.map((milestone) => milestone.title), + branches + .map((branch) => branch.name) + .filter((name) => name.startsWith("maint-"))); + if (!fixVersion) { + console.log(`No milestone found for GitHub issue #${issueInfo.number}`); + return; + } + await github.rest.issues.update({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: issueInfo.number, + milestone: milestones.find((milestone) => milestone.title === fixVersion).number + }); +} + +/** + * Closes the GitHub Issue and comments which pull request resolved it + * after that. GitHub may have closed the issue already if the pull + * request description has "Closes #XXX". + * + * @param {Object} github + * @param {Object} context + * @param {Object} pullRequest + * @param {Object} issueInfo + */ +async function closeGitHubIssue(github, context, pullRequest, issueInfo) { + const issue = { + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: issueInfo.number, + }; + if (issueInfo.state !== "closed") { + await github.rest.issues.update({...issue, state: "closed"}); + } + // The merge script may have added the same comment. + const body = `Issue resolved by pull request ${pullRequest.number}\n${pullRequest.html_url}`; + const comments = await github.paginate( + github.rest.issues.listComments, {...issue, per_page: 100}); + if (comments.some((comment) => comment.body === body)) { + return; + } + await github.rest.issues.createComment({...issue, body}); +} + +/** + * Does what is needed after a pull request is merged. + * + * @param {Object} github + * @param {Object} context + * @param {Object} pullRequest + */ +async function resolve(github, context, pullRequest) { + await clearStateLabels(github, context, pullRequest); + const issue = helpers.detectIssue(pullRequest.title); + if (!issue || issue.kind !== "github") { + return; + } + const issueInfo = await helpers.getGitHubInfo(github, context, issue.id, pullRequest.number); + if (!issueInfo) { + throw new Error(`GitHub issue #${issue.id} could not be retrieved.`); + } + await assignMilestone(github, context, pullRequest, issueInfo); + await closeGitHubIssue(github, context, pullRequest, issueInfo); +} + +/** + * This is for pushes to main. A pushed commit that isn't the merge + * commit of a pull request (e.g. a version bump by a release manager) + * is ignored. + */ +module.exports = async ({github, context}) => { + for (const commit of context.payload.commits) { + const {data: pullRequests} = + await github.rest.repos.listPullRequestsAssociatedWithCommit({ + owner: context.repo.owner, + repo: context.repo.repo, + commit_sha: commit.id + }); + for (const pullRequest of pullRequests) { + if (pullRequest.merge_commit_sha === commit.id) { + await resolve(github, context, pullRequest); + } + } + } +}; + +module.exports.detectFixVersion = detectFixVersion; +module.exports.resolve = resolve; diff --git a/.github/workflows/dev_pr_merge_check.yml b/.github/workflows/dev_pr_merge_check.yml new file mode 100644 index 000000000000..59511f608638 --- /dev/null +++ b/.github/workflows/dev_pr_merge_check.yml @@ -0,0 +1,58 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Dev PR merge check + +on: + pull_request: + branches: + - main + types: + - opened + - edited + - reopened + - synchronize + +concurrency: + group: ${{ github.workflow }}-${{ github.repository }}-${{ github.event.number }} + cancel-in-progress: true + +permissions: + contents: read + issues: read + +jobs: + merge-check: + # This is a required status check of the main branch in + # .asf.yaml: the merge button is disabled while this fails. Re-run + # this job after you fix the issue (e.g. assign it) because + # changes of the issue don't trigger this workflow. + name: Merge check + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + sparse-checkout: .github/workflows/dev_pr + + - name: Check title and issue + uses: actions/github-script@v9 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const script = require(`${process.env.GITHUB_WORKSPACE}/.github/workflows/dev_pr/merge_check.js`); + await script({github, context, core}); diff --git a/.github/workflows/dev_pr_merged.yml b/.github/workflows/dev_pr_merged.yml new file mode 100644 index 000000000000..e6feb37dbd53 --- /dev/null +++ b/.github/workflows/dev_pr_merged.yml @@ -0,0 +1,47 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Dev PR merged + +on: + push: + branches: + - main + +permissions: + contents: read + pull-requests: write + issues: write + +jobs: + resolve: + name: Resolve + if: github.repository == 'apache/arrow' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + sparse-checkout: .github/workflows/dev_pr + + - name: Resolve Issue + uses: actions/github-script@v9 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const script = require(`${process.env.GITHUB_WORKSPACE}/.github/workflows/dev_pr/merged.js`); + await script({github, context}); diff --git a/dev/README.md b/dev/README.md index 275e1ae5c92b..b29649e97358 100644 --- a/dev/README.md +++ b/dev/README.md @@ -33,8 +33,34 @@ committer. ## How to Merge a Pull Request -Please don't merge PRs using the GitHub Web interface. Instead, run -the following command: +Use the "Squash and merge" button of the GitHub Web interface. Before +you merge, check that: + +* The pull request title is what you want as the commit title. The + merge is blocked by the "Merge check" check unless the title is + `GH-XXX: [Component] Title` (or `MINOR: [Component] Title`) and + the issue `GH-XXX` is open and assigned. Re-run the "Merge check" + job after you fix the issue because changes of the issue don't + trigger it. +* The pull request description is what you want as the commit + message. Remove leftovers of the pull request template and CI + directives such as `[skip ci]` in the merge dialog if needed. + +After the merge, the "Dev PR merged" GitHub Actions workflow sets the +milestone of the issue `GH-XXX`, closes it, comments which pull +request resolved the issue and removes the +`awaiting ...` labels from the pull request. The milestone is the +milestone of the pull request: set it before you merge if the pull +request isn't released in the next major release. If the pull request +doesn't have a milestone, the milestone of the issue is kept or the +next major release is used. This is only done for pull requests to +`main`. + +### Merge script + +The merge script is still available for now. It additionally adds +`Authored-by:`, `Co-authored-by:` and `Signed-off-by:` trailers to the +commit message. Run the following command: ```bash dev/merge_arrow_pr.sh diff --git a/dev/release/post-04-website.sh b/dev/release/post-04-website.sh index 41a7ee4a04cf..05ff13fa71f0 100755 --- a/dev/release/post-04-website.sh +++ b/dev/release/post-04-website.sh @@ -75,7 +75,7 @@ rough_n_development_months=$(( git_tag=apache-arrow-${version} git_range=apache-arrow-${previous_version}..${git_tag} -committers_command_line="git shortlog -sn --group=trailer:signed-off-by ${git_range}" +committers_command_line="dev/release/utils-list-mergers.sh ${git_range}" contributors_command_line="git shortlog -sn ${git_range}" committers=$(${committers_command_line}) diff --git a/dev/release/post-10-bump-versions.sh b/dev/release/post-10-bump-versions.sh index 958a536f243b..2ed30f277c1c 100755 --- a/dev/release/post-10-bump-versions.sh +++ b/dev/release/post-10-bump-versions.sh @@ -42,6 +42,11 @@ version=$1 next_version=$2 next_version_snapshot="${next_version}-SNAPSHOT" current_version_before_bump="$(current_version)" +# The default branch requires the "Merge check" status check. So we +# can't push to the default branch directly. We need to use a pull +# request. +branch_name="bump-versions-${next_version}" +pull_request_title="MINOR: [Release] Update versions for ${next_version_snapshot}" case "${version}" in *.0.0) @@ -71,6 +76,10 @@ if [ ${BUMP_UPDATE_LOCAL_DEFAULT_BRANCH} -gt 0 ]; then git rebase upstream/${DEFAULT_BRANCH} fi +if [ ${BUMP_PUSH} -gt 0 ]; then + git checkout -b "${branch_name}" +fi + if [ ${BUMP_CHANGELOG} -gt 0 ]; then echo "Updating CHANGELOG.md for ${version}" archery release --src "${ARROW_DIR}" changelog add "${version}" @@ -94,13 +103,37 @@ if [ ${BUMP_LINUX_PACKAGES} -gt 0 ]; then fi if [ ${BUMP_PUSH} -gt 0 ]; then - echo "Pushing changes to the default branch in apache/arrow" - git push upstream ${DEFAULT_BRANCH} + echo "Pushing changes to the ${branch_name} branch in your fork" + git push -u origin "${branch_name}" + github_url=$(git remote get-url origin | \ + sed \ + -e 's,^git@github.com:,https://github.com/,' \ + -e 's,\.git$,,') + echo "Create a pull request with the following title and merge it:" + echo " ${pull_request_title}" + echo " ${github_url}/pull/new/${branch_name}" fi if [ ${BUMP_TAG} -gt 0 -a ${is_major_release} -gt 0 ]; then dev_tag=apache-arrow-${next_version}.dev - echo "Tagging ${dev_tag}" - git tag ${dev_tag} ${DEFAULT_BRANCH} - git push upstream ${dev_tag} + if [ ${BUMP_PUSH} -gt 0 ]; then + # We can't tag here because the pull request isn't merged yet. + echo "Run the following after the pull request is merged to tag ${dev_tag}:" + echo " BUMP_DEFAULT=0 BUMP_TAG=1 $0 ${version} ${next_version}" + else + echo "Tagging ${dev_tag}" + git fetch upstream ${DEFAULT_BRANCH} + # The squash merged commit of the pull request: + # "${pull_request_title} (#XXX)" + dev_commit=$(git log --format="%H %s" upstream/${DEFAULT_BRANCH} | \ + grep -F " ${pull_request_title} (#" | \ + head -n1 | \ + cut -d" " -f1) + if [ -z "${dev_commit}" ]; then + echo "The pull request isn't merged yet: ${pull_request_title}" + exit 1 + fi + git tag ${dev_tag} ${dev_commit} + git push upstream ${dev_tag} + fi fi diff --git a/dev/release/utils-list-mergers.sh b/dev/release/utils-list-mergers.sh new file mode 100755 index 000000000000..867a3f33b50d --- /dev/null +++ b/dev/release/utils-list-mergers.sh @@ -0,0 +1,65 @@ +#!/usr/bin/env bash +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Lists who merged the pull requests of the commits in a revision +# range like "git shortlog -sn" does for authors. The pull request of +# a commit is detected by the "(#XXX)" suffix of the commit title, so +# this also works for commits cherry-picked to a maintenance branch. + +set -e +set -u +set -o pipefail + +if [ "$#" -ne 1 ]; then + echo "Usage: $0 " + echo " e.g.: $0 apache-arrow-22.0.0..apache-arrow-23.0.0" + exit 1 +fi + +git_range=$1 + +: "${REPOSITORY:=apache/arrow}" +owner=${REPOSITORY%/*} +name=${REPOSITORY#*/} + +pr_numbers=$(git log --pretty=%s "${git_range}" | + { grep -o '(#[0-9]\+)$' || :; } | + { grep -o '[0-9]\+' || :; } | + sort -n -u) + +# Use GraphQL aliases to get multiple pull requests by one request. +echo "${pr_numbers}" | xargs -n 50 | while read -r batch; do + if [ -z "${batch}" ]; then + continue + fi + query="query { repository(owner: \"${owner}\", name: \"${name}\") {" + for pr_number in ${batch}; do + query+=" pr${pr_number}: pullRequest(number: ${pr_number}) {" + query+=" mergedBy { login ... on User { name } } }" + done + query+=" } }" + gh api graphql \ + -f query="${query}" \ + --jq '.data.repository[] | .mergedBy | select(.) | + if (.name // "") != "" then .name else .login end' +done | + sort | + uniq -c | + sort -k1,1nr -k2 | + awk '{count = $1; sub(/^ *[0-9]+ /, ""); printf "%6d\t%s\n", count, $0}' diff --git a/docs/source/developers/overview.rst b/docs/source/developers/overview.rst index 3b8775023236..9454b808f385 100644 --- a/docs/source/developers/overview.rst +++ b/docs/source/developers/overview.rst @@ -133,8 +133,7 @@ in the end. To make the review process smooth for everyone, try to When tests are passing and the pull request has been approved by the interested parties, a `committer `_ -will merge the pull request. This is done with a -**command-line utility that does a squash merge**. +will merge the pull request. This is done with a **squash merge**. .. dropdown:: Details on squash merge :animate: fade-in-slide-down @@ -147,9 +146,9 @@ will merge the pull request. This is done with a cherry-pick individual patches onto a maintenance branch. Your pull request will appear in the GitHub interface to have been "merged". - In the commit message of that commit, the merge tool adds the pull request - description, a link back to the pull request, and attribution to the contributor - and any co-authors. + The commit message of that commit has the pull request description, a link + back to the pull request, and attribution to the contributor and any + co-authors. .. _ai-generated-code: diff --git a/docs/source/developers/release.rst b/docs/source/developers/release.rst index fbce61ae9d17..bdbf77057dc3 100644 --- a/docs/source/developers/release.rst +++ b/docs/source/developers/release.rst @@ -105,7 +105,7 @@ assigned. archery release curate Ensure a major version milestone for a follow up release is created on GitHub. This will -automatically be used by our merge script as the new version for issues closed when +automatically be used on merging pull requests as the new version for issues closed when the maintenance branch is created. Patch Releases @@ -621,12 +621,26 @@ Be sure to go through on the following checklist: .. code-block:: Bash - # You can run the script with BUMP_TAG=0 and BUMP_PUSH=0 - # this will avoid default pushing to main and pushing the tag - # but you will require to push manually after reviewing the commits. # dev/release/post-10-bump-versions.sh 10.0.0 11.0.0 dev/release/post-10-bump-versions.sh X.Y.Z NEXT_X.NEXT_Y.NEXT_Z + This script pushes a ``bump-versions-NEXT_X.NEXT_Y.NEXT_Z`` branch to + your fork. We can't push to ``main`` directly because ``main`` requires + the "Merge check" status check. You need to create a pull request from + the branch with the title that the script shows + (``MINOR: [Release] Update versions for NEXT_X.NEXT_Y.NEXT_Z-SNAPSHOT``) + and merge it. You can run the script with ``BUMP_PUSH=0`` to review the + commits before you push them. + + For a major release, you need to tag the merged commit as + ``apache-arrow-NEXT_X.NEXT_Y.NEXT_Z.dev`` after the pull request is + merged: + + .. code-block:: Bash + + # BUMP_DEFAULT=0 BUMP_TAG=1 dev/release/post-10-bump-versions.sh 10.0.0 11.0.0 + BUMP_DEFAULT=0 BUMP_TAG=1 dev/release/post-10-bump-versions.sh X.Y.Z NEXT_X.NEXT_Y.NEXT_Z + .. dropdown:: Update docs :animate: fade-in-slide-down :class-title: sd-fs-5