Copilot commented on code in PR #8096: URL: https://github.com/apache/texera/pull/8096#discussion_r3892705393
########## .github/workflows/backport-approval-check.yml: ########## @@ -0,0 +1,195 @@ +# 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. + +# Blocks the merge while a `release/*` label on the PR has not been approved by +# that branch's release manager (.github/release-branches.yml). +# +# The point is not extra ceremony, it is that a merged PR's labels should be a +# truthful record. A label only nominates a branch, so before this check a +# manager declined a backport by staying silent — and the label stayed on, which +# read afterwards as "this shipped in 1.2" when it had not. Here a decline is an +# action: the manager either approves, or removes their label. Since the merge +# waits for every remaining label to be approved, the label set on a merged PR +# equals the set of branches it was backported to, by construction. +# +# The author's job is to ask the manager for that call, not to make it for them. +# Editing labels needs triage access, so outside contributors and Renovate or +# Dependabot PRs cannot remove a label themselves — the failure report says so +# and names who to ask. +# +# `Backport Approvals` is listed in .asf.yaml's required_status_checks, so this +# job must produce a result on EVERY pull request, or a PR that never runs it +# waits forever. That is why it is not conditioned on the title, the labels, or +# the base branch: it always runs and reports success when there is nothing to +# approve, and merge groups pass straight through (the PRs in them were already +# checked). Do not rename the job — its display name is the required context. +# +# The decision itself lives in .github/scripts/backport-gate.js, shared with +# Direct Backport Push so the pre-merge and post-merge answers cannot drift, and +# covered by .github/scripts/test_backport_gate.sh. + +name: Backport Approval Check + +on: + pull_request: + types: + - opened + - reopened + - synchronize + - labeled + - unlabeled + pull_request_review: + types: + - submitted + - dismissed + merge_group: + +# Read-only: this job never writes to the PR, and `pull_request_review` runs +# with the base repository's token even for fork PRs. +permissions: + contents: read + pull-requests: read + +concurrency: + group: backport-approvals-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + backport-approvals: + # Do not rename — this display name is the required status check context + # referenced in .asf.yaml. + name: Backport Approvals + runs-on: ubuntu-latest + steps: + - name: Skip for merge groups + if: ${{ github.event_name == 'merge_group' }} + run: | + echo "Backport approvals are checked on the pull requests themselves," + echo "before they enter the merge queue." Review Comment: This unconditional success leaves the exact race that the post-merge recheck is intended to catch. If a manager's approval is dismissed after the PR check goes green but while the PR is in the queue, this job still publishes a successful required context for the merge group; the PR can merge with the label intact, then `direct-backport-push.yml:275-279` skips that target. The merged label therefore no longer identifies a branch that received the backport, contrary to this PR's core guarantee. Resolve the PRs in the merge group and evaluate their current labels/reviews before allowing the group to merge. ########## .github/workflows/backport-approval-check.yml: ########## @@ -0,0 +1,195 @@ +# 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. + +# Blocks the merge while a `release/*` label on the PR has not been approved by +# that branch's release manager (.github/release-branches.yml). +# +# The point is not extra ceremony, it is that a merged PR's labels should be a +# truthful record. A label only nominates a branch, so before this check a +# manager declined a backport by staying silent — and the label stayed on, which +# read afterwards as "this shipped in 1.2" when it had not. Here a decline is an +# action: the manager either approves, or removes their label. Since the merge +# waits for every remaining label to be approved, the label set on a merged PR +# equals the set of branches it was backported to, by construction. +# +# The author's job is to ask the manager for that call, not to make it for them. +# Editing labels needs triage access, so outside contributors and Renovate or +# Dependabot PRs cannot remove a label themselves — the failure report says so +# and names who to ask. +# +# `Backport Approvals` is listed in .asf.yaml's required_status_checks, so this +# job must produce a result on EVERY pull request, or a PR that never runs it +# waits forever. That is why it is not conditioned on the title, the labels, or +# the base branch: it always runs and reports success when there is nothing to +# approve, and merge groups pass straight through (the PRs in them were already +# checked). Do not rename the job — its display name is the required context. +# +# The decision itself lives in .github/scripts/backport-gate.js, shared with +# Direct Backport Push so the pre-merge and post-merge answers cannot drift, and +# covered by .github/scripts/test_backport_gate.sh. + +name: Backport Approval Check + +on: + pull_request: + types: + - opened + - reopened + - synchronize Review Comment: Changing a PR's base branch emits an `edited` action, but this workflow does not subscribe to it even though the decision depends on `pr.base.ref`. A PR moved between a release branch and `main` without a new commit will not recompute this required check, potentially retaining a success calculated while approvals were not required or waiting forever for a check on the new merge state. Add `edited` to the activity types. ########## .github/scripts/backport-gate.js: ########## @@ -0,0 +1,221 @@ +// 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. + +// Decides which `release/*` labels on a pull request are cleared to backport. +// +// A label only nominates a branch; the release manager configured for it in +// .github/release-branches.yml has to have approved the PR for the fix to land +// there. Two workflows ask that same question and must never drift apart, so +// the rule lives here rather than inline in either of them: +// +// - Backport Approvals (.github/workflows/backport-approval-check.yml) asks +// it before the merge and is a required check, so a PR cannot merge while +// one of its labels is unapproved. That is what makes the label set on a +// merged PR equal to the set of branches it was backported to: declining a +// backport is removing the label, not staying silent. +// - Direct Backport Push (.github/workflows/direct-backport-push.yml) asks it +// again after the merge, immediately before pushing, and pushes only what +// is cleared. The pre-merge check cannot cover an approval dismissed in the +// window between it going green and the merge landing. +// +// Usage (JSON on stdin): +// backport-gate.js -> decision as JSON on stdout, always exit 0 +// backport-gate.js --report -> human-readable report, exit 1 if any target +// is held back (this is the required check) +// +// Input: +// { +// "entries": [{"branch": "release/v1.2", "manager": "xuang7", ...}], +// "targets": ["release/v1.2", "release/v1.3"], +// "author": "some-contributor", +// "reviews": [{"user": {"login": "xuang7"}, "state": "APPROVED"}] | null, +// } +// +// `reviews` is the PR's review list oldest-first, exactly as the REST API +// returns it. `null` means the list could not be read at all, which is kept +// distinct from an empty list: "we did not look" must never be reported as +// "nobody approved". + +"use strict"; + +// A dismissal lands on DISMISSED whatever the review was — a stale approval +// cleared by a push, or a blocking review dismissed by hand — so the state +// proves an approval is not standing, never what the dismissed review had been. +const HOLD_REASONS = { + UNCONFIGURED: (e) => + `\`${e.target}\` is not listed in .github/release-branches.yml, so no ` + + `release manager governs it — drop the label, or add the branch back to ` + + `that file`, + DISMISSED: (e) => + `@${e.manager}'s review was dismissed, so no approval stands`, + CHANGES_REQUESTED: (e) => + `@${e.manager} requested changes and has not approved since`, + UNKNOWN: (e) => + `this PR's reviews could not be read, so @${e.manager}'s approval could ` + + `not be confirmed`, + NONE: (e) => `needs an approving review from @${e.manager}`, +}; + +function holdReason(entry) { + const render = HOLD_REASONS[entry.state] || HOLD_REASONS.NONE; + return render(entry); +} + +// Latest review state per login. COMMENTED reviews never change an approval, +// and a later CHANGES_REQUESTED or DISMISSED revokes an earlier one. The REST +// API documents that reviews come back in chronological order, so the last +// state recorded for a login is the one that counts. `null` in, `null` out — +// see the note on `reviews` above. +function reviewStates(reviews) { + if (reviews === null || reviews === undefined) return null; + const states = new Map(); + for (const review of reviews) { + const login = review && review.user && review.user.login; + if (!login || review.state === "COMMENTED") continue; + states.set(login.toLowerCase(), review.state); + } + return states; +} + +function managerIndex(entries) { + return new Map( + (entries || []).map((entry) => [entry.branch, entry.manager || ""]) + ); +} + +// Split the labeled targets into the ones their release manager has signed off +// on and the ones still held back. Managers gate their own branch and nothing +// else, so approvals compose: with v1.2 and v1.3 both labeled, an approval from +// the v1.2 manager alone clears v1.2 and leaves v1.3 held back. +// Every held-back target carries the sentence explaining it, so that the +// required check and the post-merge skip comment quote the same words instead +// of each re-deriving them from the state. +function held(entry) { + return { ...entry, reason: holdReason(entry) }; +} + +function splitByApproval({ entries, targets, author, reviews }) { + const managers = managerIndex(entries); + const states = reviewStates(reviews); + const authorLogin = (author || "").toLowerCase(); + const approved = []; + const unapproved = []; + + for (const target of targets || []) { + // release-branches.yml is the authorization source, so a label naming a + // branch it does not list has nobody who could approve the backport. That + // is not the same as an entry that deliberately omits `manager`, and the + // difference is reachable: retiring a branch means dropping its entry + // entirely while its `release/*` label lives on. + if (!managers.has(target)) { + unapproved.push(held({ target, manager: "", state: "UNCONFIGURED" })); + continue; + } + const manager = managers.get(target); + if (!manager) { + approved.push({ target, manager: "", state: "UNGATED" }); + continue; + } + // GitHub does not let anyone approve their own PR, so a manager who wrote + // the fix counts as having signed off on it. Logins are case-insensitive + // while `manager` is typed by hand into the config, so fold both. + if (manager.toLowerCase() === authorLogin) { + approved.push({ target, manager, state: "AUTHORED" }); + continue; + } + const state = states ? states.get(manager.toLowerCase()) : "UNKNOWN"; + if (state === "APPROVED") { + approved.push({ target, manager, state: "APPROVED" }); + } else { + unapproved.push(held({ target, manager, state: state || "NONE" })); + } + } + + return { approved, unapproved, ok: unapproved.length === 0 }; +} + +// What the required check prints. It is the whole explanation an author gets, +// and the author may be an outside contributor or a bot that cannot edit +// labels at all (that needs triage access), so it has to name both ways out +// and say who to ask when neither is available to them. +function renderReport(decision, targets) { + const lines = []; + if (!targets || targets.length === 0) { + lines.push("No `release/*` labels on this PR — nothing to approve."); + return lines.join("\n"); + } + + const approvedNote = { + APPROVED: (e) => `approved by @${e.manager}`, + AUTHORED: (e) => `@${e.manager} is the author of this PR`, + UNGATED: () => "no release manager configured for this branch", + }; + for (const entry of decision.approved) { + const note = (approvedNote[entry.state] || approvedNote.UNGATED)(entry); + lines.push(` OK ${entry.target} — ${note}`); + } + for (const entry of decision.unapproved) { + lines.push(` BLOCK ${entry.target} — ${entry.reason}`); + } + + if (decision.ok) { + lines.unshift( + `All ${decision.approved.length} backport target(s) approved.`, + "" + ); + return lines.join("\n"); + } + + lines.unshift( + `${decision.unapproved.length} of ${targets.length} backport target(s) ` + + `not approved.`, + "" + ); + lines.push( + "", + "Every `release/*` label on this PR must be approved by that branch's", + "release manager before it can merge, so that the labels on a merged PR", + "are exactly the branches it was backported to.", + "", + "Each BLOCK above is the named manager's call, and either answer clears it:", + " - approve this PR, to send the fix to their branch; or", + " - remove the label, to decline the backport to their branch.", Review Comment: The `UNCONFIGURED` case has no named manager, yet this footer says every blocked target is a named manager's call and can be cleared by approval. That contradicts the target-specific message above, where the only resolutions are removing the label or restoring the config entry. Limit this guidance to blocks that actually name a manager. ########## .github/scripts/test_backport_gate.sh: ########## @@ -0,0 +1,190 @@ +#!/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. + +# Regression tests for backport-gate.js -- the rule deciding which release/* +# labels are cleared to backport. It is an authorization check consulted both +# before the merge (the required Backport Approvals check) and after it (Direct +# Backport Push), so a silent change of behaviour here would either block every +# merge or let an unapproved fix onto a release branch. +# +# Discovered and run automatically by the `infra` job in build.yml, which +# executes every .github/scripts/test_*.sh -- no workflow edit needed. + +set -uo pipefail + +command -v node >/dev/null || { echo "node is required to run these tests" >&2; exit 1; } + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +gate="$script_dir/backport-gate.js" +rc=0 + +pass() { echo "ok: $1"; } +failed() { echo "FAIL: $1"; rc=1; } + +# Two active branches with distinct managers, plus the retired v1.0 that is +# deliberately absent so "label with no config entry" stays covered. +entries='[{"branch":"release/v1.2","manager":"xuang7","active":true}, + {"branch":"release/v1.3","manager":"mengw15","active":true}, + {"branch":"release/v1.1","manager":"","active":false}]' + +# Runs the gate and flattens its decision to "ok|approved|held" so a case reads +# as one comparable line. Held targets carry the state that explains them, +# which is what the report turns into prose. +decide() { + node "$gate" 2>/dev/null | node -e ' + let s = ""; + process.stdin.on("data", (d) => (s += d)).on("end", () => { + const d = JSON.parse(s); + const approved = d.approved.map((e) => e.target).join(","); + const held = d.unapproved.map((e) => `${e.target}:${e.state}`).join(","); + process.stdout.write(`${d.ok ? "ok" : "blocked"}|${approved}|${held}`); + });' +} + +check() { # <description> <input-json> <expected> + local desc="$1" input="$2" want="$3" got + got="$(printf '%s' "$input" | decide)" + if [[ "$got" == "$want" ]]; then pass "$desc"; else + failed "$desc" + echo " want: $want" + echo " got: $got" + fi +} + +review() { printf '{"user":{"login":"%s"},"state":"%s"}' "$1" "$2"; } + +input() { # <author> <targets-json> <reviews-json-or-null> + printf '{"entries":%s,"author":"%s","targets":%s,"reviews":%s}' \ + "$entries" "$1" "$2" "$3" +} + +both='["release/v1.2","release/v1.3"]' + +# --- the three outcomes the flow is built around ----------------------------- +check "both managers approve -> both cleared" \ + "$(input contributor "$both" "[$(review xuang7 APPROVED),$(review mengw15 APPROVED)]")" \ + "ok|release/v1.2,release/v1.3|" + +check "only the v1.3 manager approves -> v1.3 only" \ + "$(input contributor "$both" "[$(review mengw15 APPROVED)]")" \ + "blocked|release/v1.3|release/v1.2:NONE" + +check "neither approves -> nothing cleared" \ + "$(input contributor "$both" '[]')" \ + "blocked||release/v1.2:NONE,release/v1.3:NONE" + +# --- how an approval is won, kept and lost ----------------------------------- +check "a later CHANGES_REQUESTED revokes an approval" \ + "$(input contributor '["release/v1.2"]' "[$(review xuang7 APPROVED),$(review xuang7 CHANGES_REQUESTED)]")" \ + "blocked||release/v1.2:CHANGES_REQUESTED" + +check "a dismissal revokes an approval" \ + "$(input contributor '["release/v1.2"]' "[$(review xuang7 APPROVED),$(review xuang7 DISMISSED)]")" \ + "blocked||release/v1.2:DISMISSED" + +check "a later COMMENTED review keeps the approval" \ + "$(input contributor '["release/v1.2"]' "[$(review xuang7 APPROVED),$(review xuang7 COMMENTED)]")" \ + "ok|release/v1.2|" + +check "re-approving after changes requested restores it" \ + "$(input contributor '["release/v1.2"]' "[$(review xuang7 CHANGES_REQUESTED),$(review xuang7 APPROVED)]")" \ + "ok|release/v1.2|" + +check "someone else's approval does not count" \ + "$(input contributor '["release/v1.2"]' "[$(review yicong-huang APPROVED)]")" \ + "blocked||release/v1.2:NONE" + +check "logins compare case-insensitively" \ + "$(input contributor '["release/v1.2"]' "[$(review XuAnG7 APPROVED)]")" \ + "ok|release/v1.2|" + +# --- cases that must not deadlock a PR --------------------------------------- +check "a manager who authored the PR counts as approving" \ + "$(input XuAng7 '["release/v1.2"]' '[]')" \ + "ok|release/v1.2|" + +check "an entry that omits its manager stays ungated" \ + "$(input contributor '["release/v1.1"]' '[]')" \ + "ok|release/v1.1|" + +check "no release labels at all -> nothing to approve" \ + "$(input contributor '[]' '[]')" \ + "ok||" + +# --- fail closed, and never claim more than the state proves ----------------- +check "a label with no config entry is held, not waved through" \ + "$(input contributor '["release/v1.0"]' "[$(review xuang7 APPROVED)]")" \ + "blocked||release/v1.0:UNCONFIGURED" + +check "an unreadable review list holds every gated target" \ + "$(input contributor "$both" 'null')" \ + "blocked||release/v1.2:UNKNOWN,release/v1.3:UNKNOWN" + +# --- the required check's verdict and its wording ---------------------------- +report() { printf '%s' "$1" | node "$gate" --report 2>/dev/null; } + +if report "$(input contributor "$both" "[$(review xuang7 APPROVED),$(review mengw15 APPROVED)]")" >/dev/null; then + pass "--report exits 0 when every target is approved" +else + failed "--report should exit 0 when every target is approved" +fi + +if report "$(input contributor "$both" "[$(review mengw15 APPROVED)]")" >/dev/null; then + failed "--report should exit non-zero while a target is unapproved" +else + pass "--report exits non-zero while a target is unapproved" +fi + +if report "$(input contributor '[]' '[]')" >/dev/null; then + pass "--report exits 0 on a PR with no release labels" +else + failed "--report must exit 0 on a PR with no release labels (it is a required check)" +fi + +# The report is the whole explanation an author gets, so it has to name both +# ways out and say the call belongs to the manager. +out="$(report "$(input contributor "$both" "[$(review mengw15 APPROVED)]")")" +for phrase in "approve this PR" "remove the label" "triage access" "@xuang7"; do + if [[ "$out" == *"$phrase"* ]]; then + pass "report mentions \"$phrase\"" + else + failed "report should mention \"$phrase\"" + fi +done + +# A dismissal lands on DISMISSED whatever the review had been, so the report +# must not tell readers an approval existed. +out="$(report "$(input contributor '["release/v1.2"]' "[$(review xuang7 APPROVED),$(review xuang7 DISMISSED)]")")" +if [[ "$out" == *"approved"* && "$out" != *"was dismissed"* ]]; then + failed "a dismissed review must not be reported as a standing approval" +else + pass "a dismissed review is reported without claiming an approval existed" +fi Review Comment: This assertion does not catch the wording regression it describes: the old incorrect phrase “approval was dismissed” contains both `approved` and `was dismissed`, so the condition passes it (as does the report's unavoidable “not approved” text). Assert that “approval was dismissed” is absent and that the intended “review was dismissed” wording is present. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
