Yicong-Huang commented on code in PR #8096:
URL: https://github.com/apache/texera/pull/8096#discussion_r3921544766
##########
.github/workflows/backport-auto-label.yml:
##########
@@ -205,6 +220,15 @@ jobs:
for (const entry of entries) {
const label = entry.branch;
const manager = entry.manager;
+ // Say who has to sign off, so the label is not mistaken for
the
+ // decision. A manager who wrote the fix needs no approval —
+ // GitHub does not let anyone approve their own PR.
+ const gate =
+ manager && !managerIsAuthor(manager)
+ ? ` @${manager} decides: approving sends the fix here, ` +
+ `removing this label declines it. The merge waits on one
` +
+ `or the other.`
+ : "";
// Only actively-supporting branches are analyzed /
auto-labeled.
// An inactive branch stays a valid manual target (label it by
Review Comment:
This is the mirror of `release-branches.yml:36-41`, which you corrected in
this same commit. That copy now adds "but its manager must still approve before
the PR can merge, and nothing asks them to"; this one still says only that the
apply-check and post-merge backport run.
It matters more here, because this is the workflow the claim describes.
`release_branches.py:87` returns inactive entries, so
`backport-approval-check.yml:104-110` loads `release/v1.1` with manager
`bobbai00` and `:168-189` blocks it. Meanwhile `:237-240` below `continue`s on
`!entry.active` before the `reviewRequests.push` at `:301`.
So a maintainer who reads this and hand-labels an inactive branch gets a red
required check, waiting on an approval nobody was asked for.
##########
AGENTS.md:
##########
@@ -258,5 +258,9 @@ diff -> pr-labeler -> labels on PR -> required-checks maps
labels to stacks -> C
suspect breaks the frontend)? **Add the relevant label manually**.
- Empty stack union (docs-only / dev-only / `dependencies` / `feature` /
`fix` / `refactor` / `release/*` only) skips every build stack on purpose.
-- `release/*` labels select backport targets; removing one cancels that
- backport.
+- `release/*` labels nominate backport targets. A nominated target is
+ backported only once that branch's release manager — listed in
+ [`.github/release-branches.yml`](.github/release-branches.yml) — approves the
+ PR, and the required `Backport Approvals` check blocks the merge until every
+ label on the PR is approved. A manager declines by removing their label, so
Review Comment:
"every label on the PR" drops the `release/*` qualifier. The bullet just
above (`:259-260`) enumerates `dependencies`, `feature`, `fix`, `refactor` and
`release/*`, so the unqualified phrasing reads as covering all of them.
`CONTRIBUTING.md:141`, rewritten in the same PR, says "every `release/*` label
on the PR".
```suggestion
PR, and the required `Backport Approvals` check blocks the merge until
every
`release/*` label on the PR is approved. A manager declines by removing
their
label, so
```
##########
.github/workflows/backport-approval-check.yml:
##########
@@ -0,0 +1,316 @@
+# 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 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 declining is an action: the manager
+# either approves, or removes their label. Since the merge waits for every
+# remaining label to be approved, the labels on a merged PR are exactly the
+# branches Direct Backport Push then sends the fix to.
+#
+# 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. Merge groups are re-evaluated rather than waved through, because a
+# queued PR's own checks are no longer consulted once it is queued; a group
+# whose PRs cannot be resolved fails closed rather than passing unchecked. Do
+# not rename the job — its display name is the required context.
+
+name: Backport Approval Check
+
+on:
+ pull_request:
+ types:
+ - opened
+ - reopened
+ - synchronize
+ - labeled
+ - unlabeled
+ # Retargeting arrives as `edited`, and the verdict depends on the base
+ # branch: a backport PR into release/* has nothing to approve, the same
+ # PR moved onto main does.
+ - edited
+ 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:
+ # The PR's own head (or the queued group's), so the config is the one
that
+ # will govern this change once it merges. Checking out the base would not
+ # be a security boundary anyway: on `pull_request` the workflow
definition
+ # itself comes from the PR. What bounds this is the event — a fork gets a
+ # read-only token and no secrets, and nothing here writes to the PR.
+ - name: Checkout
+ uses: actions/checkout@v7
+ with:
+ ref: ${{ github.event.pull_request.head.sha ||
github.event.merge_group.head_sha }}
+ persist-credentials: false
+
+ - name: Read release branches
+ id: targets
+ run: |
+ entries=$(python3 .github/scripts/release_branches.py
.github/release-branches.yml)
+ echo "entries=${entries}" >> "$GITHUB_OUTPUT"
+ echo "Release branch targets: ${entries}"
+
+ - name: Check release manager approvals
+ uses: actions/github-script@v9
+ env:
+ ENTRIES: ${{ steps.targets.outputs.entries }}
+ with:
+ script: |
+ const { owner, repo } = context.repo;
+ const managers = new Map(
+ JSON.parse(process.env.ENTRIES || "[]").map((e) => [
+ e.branch,
+ e.manager || "",
+ ])
+ );
+
+ // A merge group carries several PRs and its commits are the squash
+ // merges, so the number is parsed out of each subject — the same
way
+ // precheck.yml resolves them.
+ async function pullRequestNumbers() {
+ if (context.eventName !== "merge_group") {
+ return [context.payload.pull_request.number];
+ }
+ const mg = context.payload.merge_group;
+ const { data: comparison } = await
github.rest.repos.compareCommits({
+ owner, repo, base: mg.base_sha, head: mg.head_sha,
+ });
+ return comparison.commits.map((c) => {
+ const m = c.commit.message.split("\n",
1)[0].match(/\(#(\d+)\)\s*$/);
+ if (!m) throw new Error(`no PR number in the message of
${c.sha}`);
+ return Number(m[1]);
+ });
+ }
+
+ // Latest review state per login. COMMENTED never changes an
approval;
+ // a later CHANGES_REQUESTED or DISMISSED revokes one. listReviews
is
+ // documented to return reviews chronologically, so the last state
+ // recorded for a login is the one that counts. `null` means the
list
+ // could not be read at all — kept distinct from an empty list,
since
+ // "we did not look" must never be reported as "nobody approved".
+ async function reviewStates(number) {
+ try {
+ const reviews = await
github.paginate(github.rest.pulls.listReviews, {
+ owner, repo, pull_number: number, per_page: 100,
+ });
+ const states = new Map();
+ for (const review of reviews) {
+ const login = review.user?.login;
+ if (!login || review.state === "COMMENTED") continue;
+ states.set(login.toLowerCase(), review.state);
+ }
+ return states;
+ } catch (e) {
+ core.warning(
+ `listReviews for #${number} failed (${e.status ?? "?"}):
${e.message}`
+ );
+ return null;
+ }
+ }
+
+ // Managers gate their own branch and nothing else, so approvals
+ // compose: an approval from the v1.2 manager alone clears v1.2 and
+ // leaves v1.3 blocked.
+ function decide(targets, author, states) {
+ const authorLogin = (author || "").toLowerCase();
+ const cleared = [];
+ const blocked = [];
+ 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
+ // it. That differs from an entry that deliberately omits
+ // `manager`, and the difference is reachable: retiring a
branch
+ // means dropping its entry while its label lives on.
+ if (!managers.has(target)) {
+ blocked.push({ target, manager: "", state: "UNCONFIGURED" });
+ continue;
+ }
+ const manager = managers.get(target);
+ if (!manager) {
+ cleared.push({ target, note: "no release manager configured
for this branch" });
+ continue;
+ }
+ // GitHub does not let anyone approve their own PR, so a
manager
+ // who wrote the fix counts as having signed off. Logins are
+ // case-insensitive while `manager` is typed by hand, so fold
both.
+ if (manager.toLowerCase() === authorLogin) {
+ cleared.push({ target, note: `@${manager} is the author of
this PR` });
+ continue;
+ }
+ const state = states ? states.get(manager.toLowerCase()) :
"UNKNOWN";
+ if (state === "APPROVED") {
+ cleared.push({ target, note: `approved by @${manager}` });
+ } else {
+ blocked.push({ target, manager, state: state || "NONE" });
+ }
+ }
+ return { cleared, blocked };
+ }
+
+ // Say only what the state proves. A dismissal lands on DISMISSED
+ // whatever the review was — GitHub dismisses a blocking
+ // CHANGES_REQUESTED as readily as it clears a stale approval — so
+ // naming it an approval would put a guess in the manager's mouth.
+ function reason(e) {
+ if (e.state === "VETOED") {
+ return "`no-backport-needed` says this fix reaches no release
" +
+ "branch, so this label contradicts it and would be left on a
" +
+ "merged PR promising a backport nobody performs — remove the
" +
+ "label, or remove `no-backport-needed`";
+ }
+ if (e.state === "UNCONFIGURED") {
+ return "this branch 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";
+ }
+ if (e.state === "DISMISSED") {
+ return `@${e.manager}'s review was dismissed, so no approval
stands`;
+ }
+ if (e.state === "CHANGES_REQUESTED") {
+ return `@${e.manager} requested changes and has not approved
since`;
+ }
+ if (e.state === "UNKNOWN") {
+ return `this PR's reviews could not be read, so
@${e.manager}'s ` +
+ `approval could not be confirmed`;
+ }
+ return `needs an approving review from @${e.manager}`;
+ }
+
+ let numbers;
+ try {
+ numbers = await pullRequestNumbers();
+ } catch (e) {
+ // Fail closed. Only reachable for a merge group whose commits
+ // cannot be mapped back to PRs, and a group whose approvals
+ // cannot be read is not a group this can vouch for — passing it
+ // would leave the queue ungated in exactly the window this job
+ // exists to cover. Ejecting the group is recoverable: its PRs
+ // re-queue, and each still holds its own passing check.
+ core.setFailed(
+ `Could not resolve the pull requests in this merge group ` +
+ `(${e.message}). Refusing to pass a check that could not be ` +
+ `computed.`
+ );
+ return;
+ }
+
+ const lines = [];
+ let failed = false;
+
+ for (const number of numbers) {
+ const { data: pr } = await github.rest.pulls.get({
+ owner, repo, pull_number: number,
+ });
+ const labels = (pr.labels || []).map((l) => l.name);
+ const releaseLabels = [...new Set(
+ labels.filter((n) => /^release\/.+$/.test(n))
+ )].sort();
+ // A PR already targeting a release branch is itself a backport
and
+ // is not backported onward, so there is nothing here to approve.
+ const intoMain = pr.base.ref === "main";
+ // `no-backport-needed` says the fix reaches no release branch,
but
+ // nothing removes the labels it overrides — and precheck
documents
+ // adding it mid-review, by which point the auto-labeler has
already
+ // applied them. Merging in that state leaves a label promising a
+ // backport the push will skip: the false record this check
exists
+ // to prevent. So the contradiction is reported rather than
settled
+ // silently in the veto's favour.
+ const vetoed = labels.includes("no-backport-needed");
+ const targets = intoMain && !vetoed ? releaseLabels : [];
+ const contradictions =
+ intoMain && vetoed
+ ? releaseLabels.map((target) => ({
+ target, manager: "", state: "VETOED",
+ }))
+ : [];
+
+ if (numbers.length > 1) lines.push(`### PR #${number}`, "");
+ if (targets.length === 0 && contradictions.length === 0) {
+ lines.push("No `release/*` labels — nothing to approve.", "");
Review Comment:
Now that the veto case correctly blocks, this line covers two conditions,
and the residual one is `pr.base.ref !== "main"` (`:254`). A PR based on a
release branch that does carry `release/*` labels is told it has none. Worth
its own message.
##########
.github/workflows/backport-approval-check.yml:
##########
@@ -0,0 +1,316 @@
+# 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 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 declining is an action: the manager
+# either approves, or removes their label. Since the merge waits for every
+# remaining label to be approved, the labels on a merged PR are exactly the
+# branches Direct Backport Push then sends the fix to.
+#
+# 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. Merge groups are re-evaluated rather than waved through, because a
+# queued PR's own checks are no longer consulted once it is queued; a group
+# whose PRs cannot be resolved fails closed rather than passing unchecked. Do
+# not rename the job — its display name is the required context.
+
+name: Backport Approval Check
+
+on:
+ pull_request:
+ types:
+ - opened
+ - reopened
+ - synchronize
+ - labeled
+ - unlabeled
+ # Retargeting arrives as `edited`, and the verdict depends on the base
+ # branch: a backport PR into release/* has nothing to approve, the same
+ # PR moved onto main does.
+ - edited
+ 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:
+ # The PR's own head (or the queued group's), so the config is the one
that
+ # will govern this change once it merges. Checking out the base would not
+ # be a security boundary anyway: on `pull_request` the workflow
definition
+ # itself comes from the PR. What bounds this is the event — a fork gets a
+ # read-only token and no secrets, and nothing here writes to the PR.
+ - name: Checkout
+ uses: actions/checkout@v7
+ with:
+ ref: ${{ github.event.pull_request.head.sha ||
github.event.merge_group.head_sha }}
+ persist-credentials: false
+
+ - name: Read release branches
+ id: targets
+ run: |
+ entries=$(python3 .github/scripts/release_branches.py
.github/release-branches.yml)
+ echo "entries=${entries}" >> "$GITHUB_OUTPUT"
+ echo "Release branch targets: ${entries}"
+
+ - name: Check release manager approvals
+ uses: actions/github-script@v9
+ env:
+ ENTRIES: ${{ steps.targets.outputs.entries }}
+ with:
+ script: |
+ const { owner, repo } = context.repo;
+ const managers = new Map(
+ JSON.parse(process.env.ENTRIES || "[]").map((e) => [
+ e.branch,
+ e.manager || "",
+ ])
+ );
+
+ // A merge group carries several PRs and its commits are the squash
+ // merges, so the number is parsed out of each subject — the same
way
+ // precheck.yml resolves them.
+ async function pullRequestNumbers() {
+ if (context.eventName !== "merge_group") {
+ return [context.payload.pull_request.number];
+ }
+ const mg = context.payload.merge_group;
+ const { data: comparison } = await
github.rest.repos.compareCommits({
+ owner, repo, base: mg.base_sha, head: mg.head_sha,
+ });
+ return comparison.commits.map((c) => {
+ const m = c.commit.message.split("\n",
1)[0].match(/\(#(\d+)\)\s*$/);
+ if (!m) throw new Error(`no PR number in the message of
${c.sha}`);
+ return Number(m[1]);
+ });
+ }
+
+ // Latest review state per login. COMMENTED never changes an
approval;
+ // a later CHANGES_REQUESTED or DISMISSED revokes one. listReviews
is
+ // documented to return reviews chronologically, so the last state
+ // recorded for a login is the one that counts. `null` means the
list
+ // could not be read at all — kept distinct from an empty list,
since
+ // "we did not look" must never be reported as "nobody approved".
+ async function reviewStates(number) {
+ try {
+ const reviews = await
github.paginate(github.rest.pulls.listReviews, {
+ owner, repo, pull_number: number, per_page: 100,
+ });
+ const states = new Map();
+ for (const review of reviews) {
+ const login = review.user?.login;
+ if (!login || review.state === "COMMENTED") continue;
+ states.set(login.toLowerCase(), review.state);
+ }
+ return states;
+ } catch (e) {
+ core.warning(
+ `listReviews for #${number} failed (${e.status ?? "?"}):
${e.message}`
+ );
+ return null;
+ }
+ }
+
+ // Managers gate their own branch and nothing else, so approvals
+ // compose: an approval from the v1.2 manager alone clears v1.2 and
+ // leaves v1.3 blocked.
+ function decide(targets, author, states) {
Review Comment:
`decide`, `reason` and `reviewStates` live inside this `github-script`
string, where nothing can exercise them. The description says the eight cases
were driven through locally, so that coverage ended with the session.
Three concepts are also re-declared: the label→target derivation at
`:249-251` (also `precheck.yml:336-339`), the branch→manager Map at `:104-110`
(also `direct-backport-push.yml:266-271`), and the `manager == author` fold at
`:181` (also `backport-auto-label.yml:140`). Your own new comment there says
the two must agree or the check clears a branch the report is still holding —
with nothing enforcing it.
This PR had the shape earlier: `backport-gate.js` with
`test_backport_gate.sh`, dropped when the logic was inlined.
`build.yml:1291-1307` already runs every `test_*.sh` under `.github/scripts/`.
Advisory — every copy agrees today.
##########
.github/workflows/backport-approval-check.yml:
##########
@@ -0,0 +1,316 @@
+# 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 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 declining is an action: the manager
+# either approves, or removes their label. Since the merge waits for every
+# remaining label to be approved, the labels on a merged PR are exactly the
+# branches Direct Backport Push then sends the fix to.
+#
+# 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. Merge groups are re-evaluated rather than waved through, because a
+# queued PR's own checks are no longer consulted once it is queued; a group
+# whose PRs cannot be resolved fails closed rather than passing unchecked. Do
+# not rename the job — its display name is the required context.
+
+name: Backport Approval Check
+
+on:
+ pull_request:
+ types:
+ - opened
+ - reopened
+ - synchronize
+ - labeled
+ - unlabeled
+ # Retargeting arrives as `edited`, and the verdict depends on the base
+ # branch: a backport PR into release/* has nothing to approve, the same
+ # PR moved onto main does.
+ - edited
+ 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:
+ # The PR's own head (or the queued group's), so the config is the one
that
+ # will govern this change once it merges. Checking out the base would not
+ # be a security boundary anyway: on `pull_request` the workflow
definition
+ # itself comes from the PR. What bounds this is the event — a fork gets a
+ # read-only token and no secrets, and nothing here writes to the PR.
+ - name: Checkout
+ uses: actions/checkout@v7
+ with:
+ ref: ${{ github.event.pull_request.head.sha ||
github.event.merge_group.head_sha }}
+ persist-credentials: false
+
+ - name: Read release branches
+ id: targets
+ run: |
+ entries=$(python3 .github/scripts/release_branches.py
.github/release-branches.yml)
+ echo "entries=${entries}" >> "$GITHUB_OUTPUT"
+ echo "Release branch targets: ${entries}"
+
+ - name: Check release manager approvals
+ uses: actions/github-script@v9
+ env:
+ ENTRIES: ${{ steps.targets.outputs.entries }}
+ with:
+ script: |
+ const { owner, repo } = context.repo;
+ const managers = new Map(
+ JSON.parse(process.env.ENTRIES || "[]").map((e) => [
+ e.branch,
+ e.manager || "",
+ ])
+ );
+
+ // A merge group carries several PRs and its commits are the squash
+ // merges, so the number is parsed out of each subject — the same
way
+ // precheck.yml resolves them.
+ async function pullRequestNumbers() {
+ if (context.eventName !== "merge_group") {
+ return [context.payload.pull_request.number];
+ }
+ const mg = context.payload.merge_group;
+ const { data: comparison } = await
github.rest.repos.compareCommits({
+ owner, repo, base: mg.base_sha, head: mg.head_sha,
+ });
+ return comparison.commits.map((c) => {
Review Comment:
No `length === 0` check here. If `comparison.commits` is empty, `numbers` is
`[]` and the loop at `:244` never runs, so `failed` stays `false` and the job
publishes a green `Backport Approvals` for a group whose approvals were never
read.
`precheck.yml:145-147` — the code this mirrors — guards exactly that, and
the asymmetry runs the wrong way: precheck degrades by running more builds,
this one by waving the group through. It is also the residue of r3894366331;
the `core.setFailed` at `:234-239` catches the throwing branch, not the
empty-array one. The `throw` below already routes there, so one line closes it.
```suggestion
if (comparison.commits.length === 0) {
throw new Error(`no commits between ${mg.base_sha} and
${mg.head_sha}`);
}
return comparison.commits.map((c) => {
```
Advisory, not must-fix: `min_entries_to_merge: 2` means a real group always
carries commits, so I cannot name a trigger. But silent-and-green is the worst
direction for an authorization check.
--
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]
