[#12544] feat(ci): aggregate reusable suites into one Required CI check by nevzheng · Pull Request #12545 · apache/gravitino

nevzheng · 2026-08-21T02:55:17Z

What changes were proposed in this pull request?

main later gets one stable check named Required CI. This PR is the parent that fans out to the 14 public suites and ANDs their results. Suites no longer listen on pull_request; they are workflow_call + push. Path filters stay inside each suite (dorny). A skipped inner job still makes the reusable call success, so a docs-only PR can stay green.

flowchart LR
  PR["PR opened / synchronize"] --> Parent["Required CI parent"]
  Parent --> Suites["14 reusable suites"]
  Suites --> Agg["Required CI job<br/>always(); fail if any call != success"]
  Agg --> Gate{"later main ruleset"}
  Gate -->|green| Merge["merge"]
  Gate -->|red or missing| Block["block"]

Review follow-ups already in this branch: unique standalone concurrency (github.workflow, not a shared 'standalone' key), web-ui-tests has an internal web/web/** filter, parent cancel-in-progress: true, coverage sidecar listens for Required CI instead of a standalone build run, and the aggregate prints every suite result.

Why are the changes needed?

main has no required status checks, so a PR can merge on review alone when CI is red. A failing suite reports UNSTABLE, not BLOCKED (#12531). One named Required CI check is what a later ruleset can require, and it is the foundation for the CI speed epic (#3834).

Does this PR introduce any user-facing change?

No API or property changes.

Contributor-facing: PRs no longer start 14 top-level suite workflow runs. Suites show as nested checks under Required CI (build / …, spark / …). Logs are one click deeper. push to main / branch-* is unchanged (each suite still starts itself).

Coverage comments: coverage-comment.yml still posts from a sidecar (fork PRs cannot comment from the pull_request run). It now waits for Required CI, not build. Missing artifact → ACTION=skip, sidecar stays green. That listener is loaded from the default branch, so it takes effect after merge. This PR will not get a coverage comment from the new YAML.

How was this patch tested?

python3 dev/ci/test_required_ci.py — source contract only; does not start Actions. Locks parent fan-out, unique concurrency, web-ui internal filter, coverage listener name, and the skip path.
Live on this PR: nested suite checks appear under Required CI; later pushes cancel the stale parent run.
Not verified here: coverage comment posting. workflow_run workflows are loaded from main, which still listens for build. Canary is the next PR after merge.

Fix: #12544

Nevin
Sent from my 🤖 (Cursor)

Fixes DAT-643

jerryshao

Automated review pass on the Required CI aggregation. Found a critical concurrency-group regression that affects all 13 aggregated suites, plus a lost path filter, a broken PR coverage-comment trigger, a serialization issue on the parent orchestrator, and an unrelated design doc bundled into this PR. Details inline.

Additional finding not anchorable inline (.github/workflows/coverage-comment.yml is not part of this diff, so it can't be commented on directly):

correctness: this workflow_run: workflows: ["build"] trigger (guarded elsewhere by github.event.workflow_run.event == 'pull_request') will never fire again for PRs, because build.yml no longer runs as a directly-triggered top-level workflow for pull_request events — only nested via workflow_call from required-ci.yml — and GitHub Actions does not generate a matching workflow_run completion event for a workflow invoked via workflow_call.

Every future PR's build job now only executes as part of the Required CI run, not as a standalone build workflow run triggered by pull_request; the only surviving direct trigger for build.yml is push. This event == 'pull_request' condition can then never be satisfied, silently and permanently disabling the PR coverage-report comment feature. Worth noting the PR's own test plan lists this as an unverified risk — seems worth resolving before merge rather than leaving as a known gap.

…CI check Add required-ci.yml as the sole pull_request listener that fans out to the public CI suites via workflow_call and aggregates all results into one Required CI check. Each suite gains a workflow_call trigger with a required_ci input and loses its direct pull_request trigger (parent is the sole listener, avoiding duplicate runs); push triggers and path-based skips are unchanged. conflict-marker-check and asf-allowlist-check stay standalone. This gives main one enforceable check a ruleset can require, gating merges on CI and enabling auto-merge. Foundation for the CI speed epic (apache#3834). Co-authored-by: Cursor <cursoragent@cursor.com>

…ress false) The parent's cancel-in-progress: true cancelled Required CI runs mid-flight when multiple pull_request events fired for the same PR (opened/synchronize), leaving a failed Required CI check and blocking the PR. Set cancel-in-progress: false so a run, once started, completes and reports a stable aggregate. Also restrict pull_request types to [opened, synchronize, reopened] to cut redundant runs (upstream has no opt-in label). Co-authored-by: Cursor <cursoragent@cursor.com>

When called via workflow_call from required-ci.yml, all 14 child suites shared the same concurrency group (required-<PR>) with cancel-in-progress: true, so they cancelled each other mid-flight, leaving Required CI red with ~13 cancelled jobs. Add github.workflow to each child's group so they are unique per suite and no longer cancel each other. Co-authored-by: Cursor <cursoragent@cursor.com>

github.workflow is not unique under workflow_call (it evaluates to the same value for all child suites), so the prior fix still left all 14 children sharing one concurrency group with cancel-in-progress: true, cancelling each other before starting (last to start won; web_ui has no concurrency block so it ran fine). Use a literal unique key per suite (required-build, required-backend, ...) so each child's group is truly unique and suites no longer cancel each other. Co-authored-by: Cursor <cursoragent@cursor.com>

jerryshao · 2026-08-21T08:10:57Z

Seems like other CI workflow doesn't run anymore.

Lock the parent/child workflow contract without starting Actions: fourteen reusable suites, a static always-on Required CI aggregate, unique required concurrency keys, and a standalone conflict-marker check. Co-authored-by: Cursor <cursoragent@cursor.com>

Shared 'standalone' concurrency cancelled every push-to-main suite. Use github.workflow on the standalone path, restore web-ui's internal web/web/** filter, and cancel superseded parent PR runs. Co-authored-by: Cursor <cursoragent@cursor.com>

nevzheng · 2026-08-25T06:03:32Z

@jerryshao The other suite names no longer show up as their own workflows because they no longer listen on pull_request. They run as jobs under Required CI — you should still see each suite as a nested check on this PR. That is the intended shape, not a regression.

Your Aug 21 review comments are addressed in 5b48b8a:

Standalone concurrency now uses github.workflow instead of a shared 'standalone' key, so push-to-main suites stop cancelling each other.
web-ui-tests has an internal web/web/** filter again, so a Java/docs PR no longer runs the full pnpm suite.
The parent cancels superseded PR runs (cancel-in-progress: true).
The unrelated experimental SPIP is already out of this PR.

Nevin
Sent from my 🤖 (Cursor)

coverage-comment.yml waited for a standalone build run. PRs no longer start that workflow, so the Jacoco comment would never post. Listen for Required CI instead; the called build still uploads the artifact onto that parent run. Co-authored-by: Cursor <cursoragent@cursor.com>

Required CI printed only failures, so a green check hid whether a suite ran. Print every call result. When the coverage sidecar finds no artifact, log ACTION=skip and the reason instead of failing. Co-authored-by: Cursor <cursoragent@cursor.com>

nevzheng · 2026-08-25T06:32:35Z

@jerryshao On the coverage-comment finding in your Aug 21 review:

We kept the sidecar (fork PRs cannot comment from the pull_request run). It now listens for Required CI, not a standalone build run. build.yml still generates and uploads coverage-report on PRs; that artifact sits on the parent run, which is the run-id the sidecar already downloads.

If the artifact is missing (path-skip, build failed before upload), the sidecar logs ACTION=skip / REASON=no coverage-report artifact and stays green. It does not fail Required CI. A red sibling suite no longer hides a report build already uploaded.

python3 dev/ci/test_required_ci.py passed locally. It now asserts the listener is Required CI and that the skip path exists. We cannot live-canary the post on this PR: workflow_run workflows are loaded from the default branch, so this retarget takes effect after merge. The next PR is the canary — Required CI completes, Coverage Comment starts, comment appears if build uploaded the artifact.

Nevin
Sent from my 🤖 (Cursor)

nevzheng · 2026-08-25T06:41:47Z

Hi @jerryshao — I addressed your comments. Tip is d5caca4.

Unrelated experimental SPIP → dropped. It stays on [#12472] docs(design): Experimental feature support #12471.
Shared 'standalone' concurrency cancelled push-to-main suites → standalone now uses github.workflow; each suite keeps its own required-<suite> key on the Required CI path.
web-ui-tests lost web/web/** → internal changes / dorny filter; test skips when those paths miss.
Parent cancel-in-progress: false serialized stale 14-suite runs → parent is true again.
“Other CI doesn’t run anymore” → intended. Suites no longer self-listen on pull_request. They run as nested checks under Required CI, not 14 top-level workflow runs.
Coverage comment would never post (sidecar waited for a standalone build run) → it now listens for Required CI. Missing artifact logs ACTION=skip and stays green. Takes effect after merge (workflow_run is loaded from main).

Also added dev/ci/test_required_ci.py. It reads the workflow YAML and checks the contract without starting Actions: parent is the only suite pull_request listener, 14 suites stay reusable and keep push, Required CI is a static always() job that fails on any non-success, concurrency keys are unique, web-ui keeps an internal path filter, and the coverage listener name is Required CI.

Adding @jerryshao @yuqi1129 and @roryqi as reviewers. We are improving CI so main can later require one stable Required CI check instead of merging on review alone.

PTAL, thanks! If it looks good, please approve.

Nevin
Sent from my 🤖 (Cursor)

A later ruleset that requires only Required CI plus conflict-marker-check could still merge a red chart or allowlist run. Both are now reusable calls in the aggregate; skipped chart paths still report success. conflict-marker-check stays standalone. The contract script discovers yml/yaml PR listeners and runs from Required CI. Co-authored-by: Cursor <cursoragent@cursor.com>

Cancelled Required CI runs still uploaded coverage-report, and the sidecar trusted a fork-controlled PR number. Skip cancelled runs, resolve the PR from workflow_run, and post only when the live head SHA, repo, and branch match. The comment includes the SHA. Co-authored-by: Cursor <cursoragent@cursor.com>

Move skip/post logic into coverage_comment.py so unit tests can reject cancelled runs and fork PR numbers without GitHub, and keep the sidecar YAML plus jacoco_report.py on the same marker and SHA line. Co-authored-by: Cursor <cursoragent@cursor.com>

workflow_call cannot paths-ignore, so docs-assets-only PRs ran assemble and web-v2 src changes entered the full Gradle build. Restore the old skip via build_relevant.py and gate compile-check and build on it. Co-authored-by: Cursor <cursoragent@cursor.com>

The sidecar checks out trusted coverage_comment.py then downloaded the fork-controlled coverage zip into the same workspace, so a later PR could overwrite that script and run with pull-requests: write. Extract into coverage-artifact/ instead, and refuse to post markdown that lacks the coverage marker or exceeds GitHub's comment size. Push concurrency no longer evaluates workflow_call inputs, which can be undefined on branch builds. An empty changed-file list now fails closed so Required CI cannot go green with no compile or build. urlopen times out; path normalize only strips a literal ./ so .github stays intact. Co-authored-by: Cursor <cursoragent@cursor.com>

nevzheng · 2026-09-01T18:27:00Z

@jerryshao @yuqi1129 @roryqi @laserninja @bharos I addressed the review feedback. PTAL, and thanks for moving this forward.

Roku: Akshay (@laserninja) and Bharath (@bharos).

#12545 — [#12544] feat(ci): aggregate reusable suites into one Required CI check

Since last time: Jerry’s Aug 21 pass was already in. This round is Yuqi’s Aug 31 threads plus follow-up hardening:

Charts and allowlist are in the Required CI aggregate, so the later ruleset can actually gate them.
Coverage sidecar ignores cancelled/skipped runs, does not trust fork pr-number.txt, and posts only when live PR head matches. Artifact extract is isolated from the trusted sidecar script; comment body requires the coverage marker.
build restores the old docs/assets/** / web-v2/** skip via build_relevant.py.
Push concurrency no longer evaluates workflow_call inputs. Contract tests cover the skip/post and path models.

Another pass and approve if it looks right. Ready for review, not claiming ready-to-merge until Required CI is green on 418e1995e.

Nevin
Sent from my 🤖 (Cursor)

…w_call github.workflow is the caller name in a reusable workflow, so the old event_name == workflow_call guard collapsed every suite onto Required CI-<pr> and cancel-in-progress kept only one. Hardcode per-suite ids and evaluate them in the caller context. Co-authored-by: Cursor <cursoragent@cursor.com>

nevzheng · 2026-09-02T00:42:05Z

29e0d82d2 fixes the Required CI concurrency collision.

A callee still sees the caller github context: event_name stays pull_request, and github.workflow is Required CI. The old event_name == 'workflow_call' guard never fired, so every suite shared Required CI-<pr> with cancel-in-progress: true. Fifteen of sixteen jobs cancelled immediately (run 33551140739).

Each suite now hardcodes a unique group (required-build-<pr>, required-spark-<pr>, …). Verifying on run 33575217845 — 0 cancelled so far.

Nevin
Sent from my 🤖 (Cursor)

nevzheng requested a review from jerryshao August 21, 2026 03:06

nevzheng marked this pull request as ready for review August 21, 2026 03:13

jerryshao reviewed Aug 21, 2026

View reviewed changes

Comment thread design-docs/experimental-features/experimental-features.md Outdated

jerryshao reviewed Aug 21, 2026

View reviewed changes

Comment thread .github/workflows/access-control-integration-test.yml Outdated

Comment thread .github/workflows/web-ui-tests.yml

Comment thread .github/workflows/required-ci.yml

Comment thread design-docs/experimental-features/experimental-features.md Outdated

nevzheng and others added 4 commits August 21, 2026 15:42

nevzheng force-pushed the ci/required-ci-upstream-base branch from e64f3ef to 159e30f Compare August 21, 2026 07:42

nevzheng and others added 2 commits August 24, 2026 22:56

nevzheng requested a review from yuqi1129 August 25, 2026 06:06

nevzheng and others added 2 commits August 24, 2026 23:29

nevzheng requested review from jerryshao and roryqi August 25, 2026 06:41

Merge branch 'main' into ci/required-ci-upstream-base

f869561

yuqi1129 reviewed Aug 31, 2026

View reviewed changes

Comment thread .github/workflows/required-ci.yml

yuqi1129 reviewed Aug 31, 2026

View reviewed changes

Comment thread .github/workflows/coverage-comment.yml Outdated

yuqi1129 reviewed Aug 31, 2026

View reviewed changes

Comment thread .github/workflows/build.yml

yuqi1129 reviewed Aug 31, 2026

View reviewed changes

Comment thread dev/ci/test_required_ci.py

nevzheng and others added 5 commits September 1, 2026 09:03

nevzheng requested review from bharos, laserninja and yuqi1129 September 1, 2026 18:25

nevzheng self-assigned this Sep 1, 2026

Merge branch 'main' into ci/required-ci-upstream-base

60ce84f

nevzheng mentioned this pull request Sep 2, 2026

[wip] Add action lint to ci flows nevzheng/gravitino#12

Draft

Navigation Menu

Repository navigation

[#12544] feat(ci): aggregate reusable suites into one Required CI check - #12545

[#12544] feat(ci): aggregate reusable suites into one Required CI check#12545
nevzheng wants to merge 16 commits into
apache:mainfrom
nevzheng:ci/required-ci-upstream-base

nevzheng commented Aug 21, 2026 •

edited

Loading

Uh oh!

Uh oh!

jerryshao left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerryshao commented Aug 21, 2026

Uh oh!

nevzheng commented Aug 25, 2026 •

edited

Loading

Uh oh!

nevzheng commented Aug 25, 2026

Uh oh!

nevzheng commented Aug 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nevzheng commented Sep 1, 2026

Uh oh!

nevzheng commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

nevzheng commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Uh oh!

jerryshao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerryshao commented Aug 21, 2026

Uh oh!

nevzheng commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nevzheng commented Aug 25, 2026

Uh oh!

nevzheng commented Aug 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nevzheng commented Sep 1, 2026

Uh oh!

nevzheng commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nevzheng commented Aug 21, 2026 •

edited

Loading

nevzheng commented Aug 25, 2026 •

edited

Loading