jerryshao commented on code in PR #13049: URL: https://github.com/apache/gravitino/pull/13049#discussion_r4056802300
########## .github/workflows/fork-ci-status-update.yml: ########## @@ -0,0 +1,111 @@ +# 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. + +name: Update Fork CI Status + +on: + schedule: + - cron: '*/15 * * * *' + +permissions: + actions: read + checks: write + pull-requests: read + +jobs: + update: + name: Update fork CI checks + if: github.repository == 'apache/gravitino' + runs-on: ubuntu-slim + steps: + - name: Synchronize fork CI checks + uses: actions/github-script@v7 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const prs = await github.paginate( + 'GET /repos/{owner}/{repo}/pulls', + { owner: context.repo.owner, repo: context.repo.repo, state: 'open', per_page: 100 }); + + for (const pr of prs) { + if (!pr.head.repo) continue; + const owner = pr.head.repo.owner.login; + const repo = pr.head.repo.name; + let runs; + try { + runs = await github.paginate( Review Comment: This loop runs every 15 minutes over all open PRs and, per PR, does two unfiltered `github.paginate` calls: the fork's entire workflow-run history for that branch, and all check-runs on the upstream commit. `apache/gravitino` currently has ~263 open PRs, so that is at least ~526 requests per cycle and ~2000+/hour, against a `GITHUB_TOKEN` quota of 1000 requests per hour per repository. Once the quota is exhausted, the check-runs `paginate` on line 59 throws a 403 — it is outside the `try/catch` that wraps the workflow-runs call — and the whole loop aborts, so every PR after that point never gets its "Fork CI" check updated. On top of that this job runs on `ubuntu-slim`, which has a 15-minute hard limit; 263 PRs with pagination is unlikely to finish inside it. Suggestions: use the `head_sha` query parameter on the workflow-runs endpoint (drops the pagination entirely), wrap the check-runs call in `try/catch` too, and consider only processing PRs updated since the last cycle. ########## .github/workflows/fork-ci.yml: ########## @@ -0,0 +1,235 @@ +# 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. + +# Run the complete CI entry point from branch pushes in the contributor's repository. +name: Fork CI + +on: + push: + branches: ['**'] + +concurrency: + group: fork-ci-${{ github.ref }} + cancel-in-progress: true + +jobs: + eligibility: + name: Resolve fork CI eligibility + # Downstream mirrors can set this repository variable to keep the + # inherited workflow completely dormant. + if: vars.DISABLE_FORK_CI != 'true' + runs-on: ubuntu-slim Review Comment: **Blocking:** `ubuntu-slim` is an ASF org-level runner, not a GitHub-hosted label. The ASF infrastructure site documents it under "Use lightweight runners" (1 vCPU, 5GB RAM, no Docker engine, 15-minute hard limit), and it is only schedulable for repositories in the `apache` org. In a contributor's personal fork there is no runner with that label, so this job sits in "Waiting for a runner to pick up this job" until the 24h queue timeout. Since all 16 suites are `needs: eligibility`, Fork CI never executes in a fork at all, and the mirrored upstream "Fork CI" check stays `queued`/`action_required` forever — which defeats the purpose of the PR. Suggest `runs-on: ubuntu-latest` here. (`fork-ci-status.yml` and `fork-ci-status-update.yml` can keep `ubuntu-slim`, since both are guarded by `if: github.repository == 'apache/gravitino'` — though see the separate note about the 15-minute cap on the cron job.) ########## .github/workflows/fork-ci-status.yml: ########## @@ -0,0 +1,90 @@ +# 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. + +name: Fork CI Status + +on: + pull_request_target: + types: [opened, synchronize, reopened] + +permissions: + actions: read + checks: write + +jobs: + notify: + name: Notify fork CI + if: github.repository == 'apache/gravitino' + runs-on: ubuntu-slim + steps: + - name: Create fork CI check + uses: actions/github-script@v7 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const pr = context.payload.pull_request; + const owner = pr.head.repo.owner.login; + const repo = pr.head.repo.name; + const branch = pr.head.ref; + const headSha = pr.head.sha; + const workflow = 'fork-ci.yml'; + + let run; + for (let attempt = 0; attempt < 3 && !run; attempt++) { + if (attempt > 0) { + await new Promise(resolve => setTimeout(resolve, 3000)); + } + try { + const runs = await github.paginate( + 'GET /repos/{owner}/{repo}/actions/workflows/{workflow_id}/runs', + { owner, repo, workflow_id: workflow, branch, per_page: 100 }); + run = runs.find(candidate => candidate.head_sha === headSha); + } catch (error) { + core.warning(`Unable to read fork Actions: ${error.message}`); + } + } + + const detailsUrl = run + ? `https://github.com/${owner}/${repo}/actions/runs/${run.id}` + : `https://github.com/${owner}/${repo}/actions/workflows/${workflow}`; + const status = !run || run.status === 'completed' ? 'completed' : 'queued'; + const conclusion = run && run.status === 'completed' + ? run.conclusion + : (run ? undefined : 'action_required'); Review Comment: Worth documenting rather than changing: this mirrors `run.conclusion` from the fork straight into a check named "Fork CI", but `fork-ci.yml` is a file the contributor fully controls on their own branch. A contributor can replace it with a single `runs-on: ubuntu-latest` / `exit 0` job and the upstream required check turns green with zero tests executed. The Spark model has the same property, but it relies on committers knowing it — so it would be good to say plainly in the PR description (and ideally in a contributor doc) that "Fork CI" is a self-attested result and that reviewers must open the linked fork run to confirm the suites actually ran. ########## .github/workflows/build.yml: ########## @@ -2,20 +2,10 @@ name: build # Controls when the workflow will run on: - # Triggers the workflow on push or pull request events but only for the "main" branch - push: - branches: [ "main", "branch-*" ] - paths-ignore: - - 'docs/assets/**' - - 'web-v2/**' - pull_request: - branches: [ "main", "branch-*" ] - paths-ignore: - - 'docs/assets/**' - - 'web-v2/**' + workflow_call: Review Comment: All the suite workflows lose their `pull_request` / `push` triggers in this PR. The required status checks currently configured in `apache/gravitino`'s branch protection (`build / build`, `Backend Integration Test / ...`, etc.) will therefore never report again, and every open PR will sit at "Expected — Waiting for status to be reported" and be unmergeable until INFRA switches branch protection over to requiring only "Fork CI". That's a coordinated cutover rather than a code issue, but it isn't mentioned in the PR description — could you spell out the before/after branch-protection steps there (and in the linked issue), so whoever merges this knows what has to happen at the same time? ########## .github/workflows/build.yml: ########## @@ -216,36 +214,30 @@ jobs: ./gradlew "${gradle_args[@]}" - name: Fetch base branch for coverage diff - if: github.event_name == 'pull_request' - run: git fetch origin ${{ github.base_ref }} --depth=1 + if: steps.build-checkout.outputs.synced == 'true' + run: git fetch origin ${{ steps.build-checkout.outputs.base-ref }} --depth=1 - name: Generate Coverage Report id: coverage - if: github.event_name == 'pull_request' + if: steps.build-checkout.outputs.synced == 'true' run: | python3 dev/ci/jacoco_report.py \ - --base-ref "${{ github.base_ref }}" \ - --head-sha "${{ github.event.pull_request.head.sha }}" \ - --repo-url "${{ github.event.pull_request.head.repo.html_url }}" \ + --base-ref "${{ steps.build-checkout.outputs.base-ref }}" \ + --head-sha "${{ github.sha }}" \ + --repo-url "${{ github.server_url }}/${{ github.repository }}" \ --min-overall 40 \ --min-changed 60 \ --output coverage-report.md - - name: Save PR number - if: github.event_name == 'pull_request' && steps.coverage.outputs.has_reports == 'true' - run: echo "${{ github.event.pull_request.number }}" > pr-number.txt - - name: Upload Coverage Report - if: github.event_name == 'pull_request' && steps.coverage.outputs.has_reports == 'true' + if: steps.build-checkout.outputs.synced == 'true' && steps.coverage.outputs.has_reports == 'true' uses: actions/upload-artifact@v7 with: name: coverage-report - path: | - coverage-report.md - pr-number.txt + path: coverage-report.md Review Comment: **Blocking:** `pr-number.txt` is dropped from the artifact here, but `.github/workflows/coverage-comment.yml` is not touched by this PR and still does `fs.readFileSync('pr-number.txt', 'utf8')`. More fundamentally, that workflow triggers on `workflow_run: workflows: ["build"]`. With `build.yml` reduced to `workflow_call` only, it runs as a set of jobs inside the caller's run and never produces its own workflow run, so the `workflow_run` event can never fire again — the coverage comment feature goes away silently. And even if the trigger were fixed, the artifact is now produced in the contributor's fork run, which upstream `actions/download-artifact` cannot reach. Either adapt `coverage-comment.yml` (pull the artifact from the fork run, recover the PR number from the check/API) or state explicitly in the PR description that coverage comments are being retired. ########## .github/workflows/asf-allowlist-check.yml: ########## @@ -40,6 +33,7 @@ jobs: - uses: actions/checkout@v4 with: persist-credentials: false + - uses: ./.github/actions/checkout-and-sync Review Comment: The `persist-credentials: false` two lines above is deliberate, but this composite runs another `actions/checkout@v4` internally (`.github/actions/checkout-and-sync/action.yml`) without that option, so the token gets written back into `.git/config` as an `http.extraheader`. That means `apache/infrastructure-actions/allowlist-check@main` then runs against a workspace carrying credentials — exactly what the original step was avoiding. Either set `persist-credentials: false` on the composite's checkout, or add a pass-through input for it. ########## .github/workflows/fork-ci-status.yml: ########## @@ -0,0 +1,90 @@ +# 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. + +name: Fork CI Status + +on: + pull_request_target: + types: [opened, synchronize, reopened] + +permissions: + actions: read + checks: write + +jobs: + notify: + name: Notify fork CI + if: github.repository == 'apache/gravitino' + runs-on: ubuntu-slim + steps: + - name: Create fork CI check + uses: actions/github-script@v7 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const pr = context.payload.pull_request; + const owner = pr.head.repo.owner.login; Review Comment: `pr.head.repo` can be `null` when the head fork has been deleted. `fork-ci-status-update.yml:45` guards this with `if (!pr.head.repo) continue;`, but here it is dereferenced directly. On a `reopened` event for a PR whose fork is gone, github-script throws `TypeError: Cannot read properties of null`, the job fails with a red X unrelated to the contributor's code, and no "Fork CI" check is created at all. ########## .github/workflows/asf-allowlist-check.yml: ########## @@ -20,15 +20,8 @@ name: "ASF Allowlist Check" on: + workflow_call: Review Comment: Dropping `pull_request` (and `push` to `main`) means the INFRA action-allowlist gate no longer runs in `apache/gravitino` at all — only inside a contributor's fork via Fork CI, or manually via `workflow_dispatch`. Combined with the self-attestation point on `fork-ci-status.yml`, a PR that introduces an unpinned or non-allowlisted third-party action could land without the upstream check ever having run. This workflow is very cheap (a glob scan, no build), so it doesn't really belong to the load that needs to move off ASF runners. Suggest keeping the upstream `pull_request` trigger for this one. ########## .github/workflows/fork-ci.yml: ########## @@ -0,0 +1,235 @@ +# 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. + +# Run the complete CI entry point from branch pushes in the contributor's repository. +name: Fork CI + +on: + push: + branches: ['**'] + +concurrency: + group: fork-ci-${{ github.ref }} + cancel-in-progress: true + +jobs: + eligibility: + name: Resolve fork CI eligibility + # Downstream mirrors can set this repository variable to keep the + # inherited workflow completely dormant. + if: vars.DISABLE_FORK_CI != 'true' + runs-on: ubuntu-slim + outputs: + applicable: ${{ steps.resolve.outputs.applicable }} + valid: ${{ steps.resolve.outputs.valid }} + steps: + - name: Resolve repository and pull request + id: resolve + env: + GH_TOKEN: ${{ github.token }} + HEAD_BRANCH: ${{ github.ref_name }} + HEAD_OWNER: ${{ github.repository_owner }} + HEAD_REPOSITORY: ${{ github.repository }} + HEAD_SHA: ${{ github.sha }} + run: | + set -euo pipefail + + echo "applicable=false" >> "${GITHUB_OUTPUT}" + echo "valid=false" >> "${GITHUB_OUTPUT}" + + if [[ "${HEAD_REPOSITORY}" == "apache/gravitino" && \ + ("${GITHUB_REF}" == "refs/heads/main" || "${GITHUB_REF}" == refs/heads/branch-*) ]]; then + echo "applicable=true" >> "${GITHUB_OUTPUT}" + echo "base_ref=${GITHUB_REF_NAME}" >> "${GITHUB_OUTPUT}" + echo "valid=true" >> "${GITHUB_OUTPUT}" + exit 0 + fi + + repository_response="$(curl --fail-with-body --silent --show-error \ + --header "Accept: application/vnd.github+json" \ + --header "Authorization: Bearer ${GH_TOKEN}" \ + --header "X-GitHub-Api-Version: 2022-11-28" \ + "${GITHUB_API_URL}/repos/${HEAD_REPOSITORY}")" + source_repository="$(jq -r '.source.full_name // empty' <<< "${repository_response}")" + + if [[ "${HEAD_REPOSITORY}" != "apache/gravitino" && \ + "${source_repository}" != "apache/gravitino" ]]; then + echo "This repository is not in the apache/gravitino fork network; Fork CI is dormant." + exit 0 + fi + + echo "applicable=true" >> "${GITHUB_OUTPUT}" + matches="[]" + match_count=0 + for attempt in 1 2 3; do + response="$(curl --fail-with-body --silent --show-error \ + --header "Accept: application/vnd.github+json" \ + --header "Authorization: Bearer ${GH_TOKEN}" \ + --header "X-GitHub-Api-Version: 2022-11-28" \ + --get \ + --data-urlencode "state=open" \ + --data-urlencode "head=${HEAD_OWNER}:${HEAD_BRANCH}" \ + --data-urlencode "per_page=10" \ + "https://api.github.com/repos/apache/gravitino/pulls";)" + matches="$(jq -r \ + --arg head_sha "${HEAD_SHA}" \ + --arg head_repository "${HEAD_REPOSITORY}" \ + '[.[] | select(.head.sha == $head_sha and .head.repo.full_name == $head_repository)]' \ + <<< "${response}")" + match_count="$(jq -r 'length' <<< "${matches}")" + if [[ "${match_count}" == "1" ]]; then + break + fi + if [[ "${attempt}" != "3" ]]; then + sleep 3 + fi + done + + if [[ "${match_count}" != "1" ]]; then + echo "::error::Expected one open apache/gravitino pull request for ${HEAD_REPOSITORY}:${HEAD_BRANCH} at ${HEAD_SHA}, found ${match_count}." + exit 0 + fi + + echo "base_ref=$(jq -r '.[0].base.ref' <<< "${matches}")" >> "${GITHUB_OUTPUT}" + echo "valid=true" >> "${GITHUB_OUTPUT}" + + - name: Reject an unmatched Apache fork branch Review Comment: **Blocking:** when no single matching open PR is found, the resolve step writes `applicable=true, valid=false` and `exit 0`, and this step then fails the whole run. That makes the ordinary contributor flow fail by construction: - The normal sequence is *push branch to fork*, **then** *open the PR*. At push time there is no open PR upstream, so `match_count=0` → red X plus a failure email. Opening the PR does not emit a new `push` event, so Fork CI never re-runs — `fork-ci-status.yml` mirrors that failed run to the upstream check, and the contributor has to push an empty commit to recover. - A contributor syncing their fork's `main` from upstream pushes to `fork/main`, which has no PR → red X on every fork sync. - Upstream is affected too: `cherry-pick-branch.yml:93` does `git push origin cherry-pick-...` and only creates the PR in a later step, so every automated cherry-pick produces a failing Fork CI run on `apache/gravitino`. Could this be a no-op instead (exit success, suites skipped) when no PR is found, so only genuinely broken states are reported as failures? ########## .github/actions/checkout-and-sync/action.yml: ########## @@ -0,0 +1,150 @@ +# 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. + +name: Checkout and sync +description: Check out the upstream base branch and merge the contributor branch for fork CI. + +outputs: + base-ref: + description: Upstream branch used as the comparison base. + value: ${{ steps.base.outputs.base_ref }} + base-sha: + description: Upstream commit checked out before merging contributor changes. + value: ${{ steps.upstream.outputs.base_sha }} + head-sha: + description: Commit tested after contributor changes are merged. + value: ${{ steps.head.outputs.head_sha }} + synced: + description: Whether contributor changes were merged onto the upstream base. + value: ${{ steps.base.outputs.should_sync }} + +runs: + using: composite + steps: + - name: Resolve pull request base branch + id: base + shell: bash + env: + EVENT_BASE_REF: ${{ github.base_ref }} + GH_TOKEN: ${{ github.token }} + HEAD_OWNER: ${{ github.repository_owner }} + HEAD_BRANCH: ${{ github.ref_name }} + HEAD_REPOSITORY: ${{ github.repository }} + HEAD_SHA: ${{ github.sha }} + run: | + set -euo pipefail + + if [[ "${GITHUB_REPOSITORY}" == "apache/gravitino" && \ + ("${GITHUB_REF}" == "refs/heads/main" || "${GITHUB_REF}" == refs/heads/branch-*) ]]; then + echo "base_ref=${GITHUB_REF_NAME}" >> "${GITHUB_OUTPUT}" + echo "should_sync=false" >> "${GITHUB_OUTPUT}" + exit 0 + fi + + repository_response="$(curl --fail-with-body --silent --show-error \ + --header "Accept: application/vnd.github+json" \ + --header "Authorization: Bearer ${GH_TOKEN}" \ + --header "X-GitHub-Api-Version: 2022-11-28" \ + "${GITHUB_API_URL}/repos/${HEAD_REPOSITORY}")" + source_repository="$(jq -r '.source.full_name // empty' <<< "${repository_response}")" + + if [[ "${HEAD_REPOSITORY}" != "apache/gravitino" && \ + "${source_repository}" != "apache/gravitino" ]]; then + echo "base_ref=${EVENT_BASE_REF:-${GITHUB_REF_NAME}}" >> "${GITHUB_OUTPUT}" + echo "should_sync=false" >> "${GITHUB_OUTPUT}" + exit 0 + fi + + response="[]" + match_count=0 + for attempt in 1 2 3; do + response="$(curl --fail-with-body --silent --show-error \ + --header "Accept: application/vnd.github+json" \ + --header "Authorization: Bearer ${GH_TOKEN}" \ + --header "X-GitHub-Api-Version: 2022-11-28" \ + --get \ + --data-urlencode "state=open" \ + --data-urlencode "head=${HEAD_OWNER}:${HEAD_BRANCH}" \ + --data-urlencode "per_page=10" \ + "https://api.github.com/repos/apache/gravitino/pulls";)" + match_count="$(jq -r \ + --arg head_sha "${HEAD_SHA}" \ + --arg head_repository "${HEAD_REPOSITORY}" \ + '[.[] | select(.head.sha == $head_sha and .head.repo.full_name == $head_repository)] + | length' \ + <<< "${response}")" + if [[ "${match_count}" == "1" ]]; then + break + fi + if [[ "${attempt}" != "3" ]]; then + sleep 3 + fi + done + + if [[ "${match_count}" != "1" ]]; then Review Comment: This composite is added to roughly 60 jobs once the backend/spark/flink matrices expand, and each one independently re-queries GitHub for "exactly one open PR matching this head SHA", failing with `exit 1` if it doesn't find it. Two consequences: - A full Fork CI run takes tens of minutes to hours. If the PR is closed or merged in the meantime, jobs scheduled after that point fail with `Expected one open apache/gravitino pull request ... found 0`, and Required CI goes red for a reason unrelated to the code. - Each job merges the contributor branch onto whatever the base branch tip is *at that moment*, so different suites in the same run can be testing different trees when the base branch moves. Would it work to resolve `base_ref` / `base_sha` once in the `eligibility` job and pass them to each suite through `workflow_call` inputs, leaving the composite responsible only for checkout + merge? That also cuts the API traffic by ~60x. -- 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]
