jerryshao commented on code in PR #12545: URL: https://github.com/apache/gravitino/pull/12545#discussion_r3828072624
########## .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: **correctness:** this concurrency group is keyed only by PR number (`required-ci-${{ github.event.pull_request.number || github.ref }}`), and paired below with `cancel-in-progress: false`. So consecutive pushes to the same open PR serialize full ~14-suite CI runs instead of cancelling the stale one. A contributor pushes twice in quick succession; the second push's Required CI run is queued and has to wait for the first (now-superseded) commit's full run to finish — potentially tens of minutes to an hour — before it even starts, delaying feedback on the latest commit and burning CI compute on an already-obsolete one. Given every child suite already cancels in-progress runs on new pushes (per the concurrency-group changes above), it seems inconsistent for the parent orchestrator not to. ########## .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: **correctness:** all 13 modified suite workflows (this one and backend-integration-test, build, contrib-catalog-test, flink, frontend, iceberg-rest-trino, idp-basic, maintenance, mcp, python, spark, trino) use the identical literal string `'standalone'` as the false-branch of this concurrency-group ternary, replacing the previously-unique `github.workflow` key. On any push to `main`/`branch-*` (or `workflow_dispatch`), `inputs.required_ci` is falsy, so every one of these 13 workflows resolves its concurrency group to the same string `standalone-refs/heads/<branch>`. Since each keeps `cancel-in-progress: true`, GitHub Actions treats them as one shared queue and cancels all but the most recently started run — silently cancelling most of build/backend/spark/flink/trino/etc. on every merge to main. This reintroduces, at push-time scope, the exact cross-suite cancellation bug this PR's own last two commits were written to fix for the PR flow. Suggest keying the standalone branch on `github.workflow` (as before) rather than a shared literal, e.g. `${{ inputs.required_ci && 'required-access-control' || github.workflow }}-...`. ########## design-docs/experimental-features/experimental-features.md: ########## @@ -0,0 +1,310 @@ +<!-- Review Comment: **scope:** this 310-line SPIP design doc (an experimental-feature-gate framework) doesn't exist on `main` and is unrelated to the Required CI aggregation this PR is titled and scoped around. Bundling it in means reviewers focused on validating the CI aggregation logic are unlikely to give this draft design doc the scrutiny a standalone PR would get, and the two unrelated concerns can't be reviewed, approved, or reverted independently of each other once merged together. Worth splitting into its own PR. ########## .github/workflows/web-ui-tests.yml: ########## @@ -1,16 +1,18 @@ name: Web UI Tests on: Review Comment: **correctness:** this workflow's `pull_request` trigger (previously gated by `paths: web/web/**`) is replaced by `workflow_call`, which doesn't support path filtering — and unlike all 13 other suites here, `web-ui-tests.yml` has no internal `changes`/`dorny/paths-filter` job to compensate. A PR that only touches, say, Java catalog code or docs never touches `web/web/**`; previously this workflow simply wouldn't run. Now `required-ci.yml`'s `web_ui` job calls it unconditionally on every PR, so the full pnpm install/lint/prettier/`test:coverage`/build sequence runs on every single PR regardless of whether frontend files changed — contradicting this PR's own stated goal that a PR touching only docs no-ops every suite, and adding real CI time/cost to every PR. -- 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]
