yuqi1129 commented on code in PR #12545: URL: https://github.com/apache/gravitino/pull/12545#discussion_r3891379165
########## .github/workflows/required-ci.yml: ########## @@ -0,0 +1,187 @@ +# +# 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. +# +# The workflow `name` is a contract: coverage-comment.yml's workflow_run +# listener matches `Required CI`. Do not rename it without updating that file. +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 }} + # Cancel the superseded PR run. Children already cancel in-progress; the + # parent must too or a second push waits out a full stale 14-suite run. + cancel-in-progress: true + +jobs: + build: + uses: ./.github/workflows/build.yml + with: + required_ci: true + secrets: inherit + + backend: + uses: ./.github/workflows/backend-integration-test.yml + with: + required_ci: true + secrets: inherit + + spark: + uses: ./.github/workflows/spark-integration-test.yml + with: + required_ci: true + secrets: inherit + + flink: + uses: ./.github/workflows/flink-integration-test.yml + with: + required_ci: true + secrets: inherit + + trino: + uses: ./.github/workflows/trino-integration-test.yml + with: + required_ci: true + secrets: inherit + + iceberg_rest_trino: + uses: ./.github/workflows/iceberg-rest-trino-integration-test.yml + with: + required_ci: true + secrets: inherit + + python: + uses: ./.github/workflows/python-integration-test.yml + with: + required_ci: true + secrets: inherit + + frontend: + uses: ./.github/workflows/frontend-integration-test.yml + with: + required_ci: true + secrets: inherit + + access_control: + uses: ./.github/workflows/access-control-integration-test.yml + with: + required_ci: true + secrets: inherit + + idp_basic: + uses: ./.github/workflows/idp-basic-test.yml + with: + required_ci: true + secrets: inherit + + mcp: + uses: ./.github/workflows/mcp-integration-test.yml + with: + required_ci: true + secrets: inherit + + maintenance: + uses: ./.github/workflows/maintenance-integration-test.yml + with: + required_ci: true + secrets: inherit + + contrib_catalog: + uses: ./.github/workflows/contrib-catalog-test.yml + with: + required_ci: true + secrets: inherit + + web_ui: + uses: ./.github/workflows/web-ui-tests.yml + with: + required_ci: true + secrets: inherit + + # Aggregate every reusable suite into one check. `if: always()` runs even + # when an upstream suite is skipped (path filter), because a skipped suite + # reports `success` and must not fail the aggregate. The job fails only if + # any suite reports a non-success result. + required_ci: + name: Required CI + if: always() + needs: + - build + - backend + - spark + - flink + - trino + - iceberg_rest_trino + - python + - frontend + - access_control + - idp_basic + - mcp + - maintenance + - contrib_catalog + - web_ui Review Comment: Blocking: `Test Charts` is still triggered separately by `.github/workflows/chart-test.yaml` and is not among these calls or aggregate needs. `asf-allowlist-check` is also outside. The follow-up ruleset in #12544 says it will require only `Required CI` plus `conflict-marker-check`, so a chart or allowlist failure would still be mergeable and the original red-CI gap remains. Please either make these always-reporting reusable jobs and include them in the aggregate, or explicitly include equivalent always-present checks in the ruleset. In particular, a path-filtered standalone workflow cannot simply be required because its check stays pending when the workflow is skipped. ########## .github/workflows/coverage-comment.yml: ########## @@ -1,19 +1,35 @@ name: Coverage Comment +# Sidecar that posts the Jacoco coverage comment. It must stay a separate +# workflow: a pull_request from a fork gets a read-only GITHUB_TOKEN, so the +# Required CI / build run cannot comment. workflow_run runs on the base repo +# with write permission and does not check out PR code. +# +# Listen for the PR entry point, not `build`. After Required CI aggregation, +# build.yml is only workflow_call on PRs; GitHub does not emit workflow_run +# for a called workflow. Artifacts uploaded inside that call belong to the +# caller run, so download uses this workflow_run.id (the Required CI run). +# +# The workflow name below must match required-ci.yml `name: Required CI`. +# workflow_run workflows are loaded from the default branch, so this retarget +# takes effect after merge; this PR itself still uses main's old listener. on: workflow_run: - workflows: ["build"] + workflows: ["Required CI"] types: [completed] permissions: + actions: read pull-requests: write jobs: comment: runs-on: ubuntu-latest - if: > - github.event.workflow_run.event == 'pull_request' && - github.event.workflow_run.conclusion == 'success' + # Any completed PR run of Required CI. Do not require conclusion == + # success: a red sibling suite (spark, etc.) must not hide a coverage + # report that build already uploaded. If build skipped or failed before + # upload, the download step fails and we post nothing. + if: github.event.workflow_run.event == 'pull_request' Review Comment: Correctness/security: this accepts every completed run, including cancelled superseded runs. `cancel-in-progress: true` makes those normal, and run 32459857246 is a concrete cancelled Required CI run that still produced `coverage-report`. The sidecar would therefore overwrite the comment with stale data. It also trusts the PR number from a fork-controlled artifact. Please reject cancelled or stale runs and validate that the candidate PR current head SHA, repo, and branch match `workflow_run.head_sha`, `head_repository`, and `head_branch` before posting; also include the SHA in the comment. ########## .github/workflows/build.yml: ########## @@ -2,20 +2,22 @@ name: build # 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: Review Comment: Path-filter regression: the old `pull_request` trigger ignored `docs/assets/**` and `web-v2/**`, but `workflow_call` has no equivalent inner filter. When `source_changes` is false, `compile-check` runs `./gradlew assemble`, so docs-assets-only PRs now build instead of no-op; many `web-v2` changes also enter build filtering. Please add a `build_relevant_changes` output that preserves the old ignore semantics and gate the `compile-check` and `build` jobs on it. ########## dev/ci/test_required_ci.py: ########## @@ -0,0 +1,292 @@ +#!/usr/bin/env python3 +# 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. + +"""Check the Required CI parent/child contract without starting Actions. + +This script does not contact GitHub. It reads the checked-in workflow +sources and asserts the contract this PR relies on: + +- the parent is the sole pull_request listener for the aggregated suites +- each suite is reusable via workflow_call + required_ci and keeps push +- Required CI is a static always() aggregate that fails on any non-success +- conflict-marker-check stays standalone and is not part of the aggregate +- required-mode concurrency keys are unique per suite +- standalone (push) keys use github.workflow, not a shared 'standalone' literal +- the parent cancels superseded PR runs +- web-ui path-filters inside the called workflow +- coverage-comment listens for the Required CI parent, not a standalone build run +""" + +import re +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parents[2] +WORKFLOW_DIR = REPO_ROOT / ".github" / "workflows" +REQUIRED_CI_WORKFLOW = WORKFLOW_DIR / "required-ci.yml" +CONFLICT_MARKER_WORKFLOW = WORKFLOW_DIR / "conflict-marker-check.yml" +COVERAGE_COMMENT_WORKFLOW = WORKFLOW_DIR / "coverage-comment.yml" + +SUITE_WORKFLOWS = { Review Comment: This hardcoded inventory misses `.github/workflows/chart-test.yaml` and does not classify `asf-allowlist-check`, so the test passes even though Required CI is not the complete gate. Please discover both `*.yml` and `*.yaml` pull-request workflows dynamically and require every one to be aggregated or explicitly allowlisted. Also wire this script into CI; currently nothing invokes `test_required_ci.py`, so it cannot prevent regressions. -- 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]
