nevzheng commented on code in PR #12545:
URL: https://github.com/apache/gravitino/pull/12545#discussion_r3849993246
##########
.github/workflows/access-control-integration-test.yml:
##########
@@ -2,14 +2,19 @@ name: Access Control Integration Test
# Controls when the workflow will run
on:
- # Triggers the workflow on push or pull request events but only for the
"main" branch
+ # Triggered by the Required CI orchestrator (workflow_call) or by pushes.
+ workflow_call:
+ inputs:
+ required_ci:
+ description: Run this suite as part of the Required CI orchestrator.
+ required: false
+ type: boolean
+ default: false
push:
branches: [ "main", "branch-*" ]
- pull_request:
- branches: [ "main", "branch-*" ]
concurrency:
- group: ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.ref }}
+ group: ${{ inputs.required_ci && 'required-access-control' || 'standalone'
}}-${{ github.event.pull_request.number || github.ref }}
Review Comment:
Agreed — the shared `'standalone'` literal put every push-to-main suite in
one group and cancelled all but the last starter.
Fixed in 5b48b8af1 with your suggested form: `required-<suite> ||
github.workflow`.
- Required path keeps the unique `required-*` literals. Inside a called
workflow `github.workflow` is the caller name (`Required CI`), so using it
there would collapse all 14 suites again.
- Standalone (push / dispatch) uses `github.workflow` because the child is
then the entry point, so each suite gets its own name.
`dev/ci/test_required_ci.py` now rejects a shared `'standalone'` key.
Nevin
Sent from my 🤖 (Cursor)
##########
.github/workflows/web-ui-tests.yml:
##########
@@ -1,16 +1,18 @@
name: Web UI Tests
on:
Review Comment:
Agreed — `workflow_call` has no `paths:` filter, so the parent was running
the full pnpm suite on every PR.
Fixed in 5b48b8af1: `web-ui-tests.yml` now has the same internal `changes` /
`dorny/paths-filter` job as the other suites (`web/web/**` plus this workflow
file). `test` skips when those paths miss. The reusable call still succeeds, so
Required CI stays green on docs/Java-only PRs.
Also added the same concurrency shape (`required-web-ui` ||
`github.workflow`).
Nevin
Sent from my 🤖 (Cursor)
##########
.github/workflows/required-ci.yml:
##########
@@ -0,0 +1,179 @@
+#
+# 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.
+#
+
+# Aggregates the reusable CI suites into one stable `Required CI` check that a
+# `main` ruleset can require, so merges are gated on CI and not on review
+# alone. This parent is the sole pull-request listener; each child suite is
+# invoked via `workflow_call` and keeps its own path-based skip, so a PR that
+# touches only docs no-ops every suite but still reports a green `Required CI`.
+# Each called workflow also appears as its own check in the PR checks panel,
+# so contributors keep per-suite visibility. `push` triggers on the children
+# are unchanged, so branch builds behave exactly as before.
+#
+# Rollout is staged: land this workflow (no ruleset yet) -> canary on a few PRs
+# -> enable the `main` ruleset requiring `Required CI` (and the standalone
+# `conflict-marker-check`). Fully reversible: delete this file and restore the
+# children's `pull_request:` trigger.
+name: Required CI
+
+on:
+ pull_request:
+ branches: [main, branch-*]
+ # Minimal event set: a new PR, a new push to an open PR, or a reopened PR.
+ # Excluding labeled/unlabeled/edited avoids redundant runs (upstream has
+ # no opt-in label; every PR runs CI).
+ types: [opened, synchronize, reopened]
+
+concurrency:
+ group: required-ci-${{ github.event.pull_request.number || github.ref }}
Review Comment:
Agreed — `cancel-in-progress: false` serialized a full 14-suite run behind a
stale push.
Fixed in 5b48b8af1: parent is `cancel-in-progress: true` again. The earlier
`false` was avoiding a mid-flight red check; that was the wrong tradeoff. A
superseded PR run should cancel so the latest commit starts immediately.
Children already cancel in-progress; the parent has to as well.
Nevin
Sent from my 🤖 (Cursor)
--
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]
