Yicong-Huang commented on code in PR #8379:
URL: https://github.com/apache/texera/pull/8379#discussion_r3935648151
##########
.github/workflows/direct-backport-push.yml:
##########
@@ -571,6 +573,17 @@ jobs:
log "new_sha=${new_sha}"
echo "new_sha=${new_sha}" >> "$GITHUB_OUTPUT"
+ - name: Run Required Checks on the pushed release branch
+ if: success()
+ env:
+ GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+ TARGET_BRANCH: ${{ matrix.target }}
+ run: |
+ # The GITHUB_TOKEN push above starts no push-triggered workflows;
+ # dispatch the run the release branch would otherwise have gotten.
+ gh workflow run required-checks.yml \
+ --repo "${GITHUB_REPOSITORY}" --ref "refs/heads/${TARGET_BRANCH}"
Review Comment:
**Advisory:**
This passes `--ref "refs/heads/${TARGET_BRANCH}"`, while the dispatch API
documents `ref` as a branch or tag name and `TARGET_BRANCH` holds the bare name
everywhere else in this job (lines 398, 505). I could not confirm the API
accepts the qualified form without dispatching against the org, so this is a
question. If it is rejected, the finding above becomes an every-run failure
rather than a transient one.
##########
.github/workflows/required-checks.yml:
##########
@@ -22,6 +22,9 @@ on:
branches:
- 'main'
- 'release/**'
+ # The backport fast path pushes release branches with GITHUB_TOKEN, which
+ # starts no push-triggered runs; it dispatches this workflow instead.
+ workflow_dispatch:
Review Comment:
**Must fix:**
This is the second `workflow_dispatch:` in the same `on:` mapping; the first
is at line 36. GitHub rejects the redefined key, so run `33774668467` on this
head is named with the raw file path, concluded `failure`, and produced zero
jobs. `Required Checks` is a required context in both rulesets, so it never
reports and the PR cannot leave the merge queue. It was also not needed: base
`main` and all three release branches already declare the trigger.
```suggestion
```
If the comment is worth keeping, move it above line 36.
##########
.asf.yaml:
##########
@@ -69,20 +69,76 @@ github:
rebase: false
rulesets:
- - name: Merge Queue
+ # Rule-for-rule identical to "Merge Queue" below; split out so the bypass
+ # here stays off main. The bypass exempts actions performed as the GitHub
+ # Actions app — i.e. any workflow's GITHUB_TOKEN, which is what
+ # direct-backport-push.yml's fast path pushes with (#8377). It cannot be
+ # scoped to a single workflow. People and PATs still face every rule.
+ #
+ # Listed BEFORE "Merge Queue" deliberately: asfyaml applies rulesets in
+ # file order, so this one is created before that one stops covering the
+ # release branches. If GitHub rejects this ruleset, the apply aborts with
+ # the old protections fully intact; the failure order never leaves the
+ # release branches uncovered.
+ - name: "Merge Queue (release)"
target: branch
enforcement: active
conditions:
ref_name:
exclude: []
include:
- - "~DEFAULT_BRANCH"
# Merge queue rules do NOT support wildcard ref patterns, so
# release branches must be listed explicitly (not release/*).
# Add each release line here as it is cut.
- "refs/heads/release/v1.1"
- "refs/heads/release/v1.2"
- "refs/heads/release/v1.3"
+ bypass_actors:
+ # The GitHub Actions app.
+ - actor_id: 15368
+ actor_type: Integration
+ bypass_mode: always
+ rules:
Review Comment:
**Advisory:**
The split copies the whole merge-queue `rules:` block: five rule types,
thirteen parameters, three required contexts, byte-equivalent to lines 142-170.
They match today, and both sites cross-reference each other in comments. What
is missing is anything enforcing that: the next parameter change lands in one
copy, and `main` and the release branches then enforce different gates.
Would a YAML anchor work (`&merge_queue_rules` / `*merge_queue_rules`)? It
resolves before asfyaml sees the document, though whether its loader preserves
anchors is the part I cannot confirm.
##########
.github/workflows/direct-backport-push.yml:
##########
@@ -571,6 +573,17 @@ jobs:
log "new_sha=${new_sha}"
echo "new_sha=${new_sha}" >> "$GITHUB_OUTPUT"
+ - name: Run Required Checks on the pushed release branch
Review Comment:
**Must fix:**
This step has no `continue-on-error` and sits after the push has landed
(`new_sha` at line 572). A non-zero `gh workflow run` therefore skips the
success annotation at line 587 and fires the failure one at line 690, marking
commit status `failure` and commenting "Backport to `<target>` failed" while
the commit is on the branch. Lines 615-618 state the opposite principle for
every other post-push call.
`continue-on-error: true`, or moving the step after the success annotation,
restores it. `if: success()` at line 577 is redundant with the step default.
##########
.github/workflows/direct-backport-push.yml:
##########
@@ -22,7 +22,8 @@ on:
- main
permissions:
- actions: read
+ # write: the fast path dispatches Required Checks after its push (below).
+ actions: write
Review Comment:
**Advisory:**
Only the new dispatch step in `push-backports` needs `actions: write`, but
declaring it here grants it to `discover` and `open-backport-pr` too. The scope
also covers cancelling runs and deleting artifacts and logs. A job-level
`permissions:` block on `push-backports` scopes it to where it is used.
##########
.asf.yaml:
##########
@@ -69,20 +69,76 @@ github:
rebase: false
rulesets:
- - name: Merge Queue
+ # Rule-for-rule identical to "Merge Queue" below; split out so the bypass
Review Comment:
**Advisory:**
The PR-level gates ask a CI or workflow change to link a proof run, usually
on a fork. The description explains why the ruleset half cannot be proven
pre-merge, and that reasoning holds. The workflow half is fork-provable, and a
fork run of `Required Checks` is exactly what would have surfaced the startup
failure above.
--
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]
