yuqi1129 commented on code in PR #13049: URL: https://github.com/apache/gravitino/pull/13049#discussion_r4348025911
########## .github/scripts/update-fork-ci.cjs: ########## @@ -0,0 +1,133 @@ +/* + * 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. + */ +'use strict'; +const { createService } = require('./fork-ci-status.cjs'); + +const RECENT_MS = 6 * 60 * 60 * 1000; +// Results that change only with a new push or a trusted run, both of which signal a refresh. +const SETTLED = ['success', 'action_required']; + +async function update({ github, context, core }) { + const signal = context.eventName === 'workflow_run' && context.payload.workflow_run.name === 'Fork CI PR Signal'; + const trustedSignal = context.eventName === 'workflow_run' && + context.payload.workflow_run.event === 'workflow_dispatch' && + /^Trusted Fork CI #([1-9][0-9]*)$/.exec(context.payload.workflow_run.display_title || ''); + const manual = context.eventName === 'workflow_dispatch' || !!trustedSignal || signal; + const requested = context.payload.inputs?.['pull-request'] || (trustedSignal && trustedSignal[1]); + if (requested && !/^[1-9][0-9]*$/.test(requested)) throw new Error('Invalid PR number.'); + let prs = requested + ? [(await github.rest.pulls.get({ ...context.repo, pull_number: Number(requested) })).data] + : await github.paginate(github.rest.pulls.list, { ...context.repo, state: 'open', per_page: 100 }); + if (signal) prs = prs.filter(pr => pr.head.sha === context.payload.workflow_run.head_sha); + const now = Date.now(); + // Never restore decisions or scheduling state from Actions caches: PR code dispatched + // on the default branch can write those caches using its runner runtime token. + const service = createService(github); + const availableBases = new Map(); + const rate = (await github.rest.rateLimit.get()).data.resources.core; + // Bound work even while settled successes are watched. Rotate the starting PR to avoid starving + // older PRs, and leave quota for signal workflows and other repository automation. + const budget = manual ? Math.max(0, rate.remaining - 100) : Math.min(60, Math.max(0, rate.remaining - 100)); + let calls = 0; + github.hook.before('request', () => { calls++; }); + const failures = []; + async function latestCheck(pr) { + const checks = await github.paginate(github.rest.checks.listForRef, { + ...context.repo, ref: pr.head.sha, check_name: 'Fork CI', per_page: 100, filter: 'all' + }); + return checks.filter(check => check.app?.slug === 'github-actions').sort((a, b) => b.id - a.id)[0]; + } + async function refresh(pr, known) { + // Release branches without this CI migration keep their own pull_request workflows. + if (!availableBases.has(pr.base.sha)) { + availableBases.set(pr.base.sha, github.rest.repos.getContent({ ...context.repo, + path: '.github/workflows/fork-ci.yml', ref: pr.base.sha }).then(() => true, error => { + if (error.status === 404) return false; + throw error; + })); + } + if (!(await availableBases.get(pr.base.sha))) return; + const latest = known === undefined ? await latestCheck(pr) : known; + const mirror = await service.mirror(pr); + const { result, run } = mirror; + const note = result.note || (result.status !== 'completed' ? 'Waiting for the fork run.' + : result.conclusion === 'success' ? 'Verified all caller suites.' : 'Open the run to see which jobs did not succeed.'); + const testedBase = mirror.base && mirror.base !== pr.base.sha ? `\nTested base: ${mirror.base}` : ''; + const record = { owner: mirror.owner, repo: mirror.repo, run_id: run?.id, + run_attempt: run?.run_attempt, base_ref: pr.base.ref, base_sha: pr.base.sha, trusted: mirror.trusted || false }; + const details = run ? `https://github.com/${mirror.owner}/${mirror.repo}/actions/runs/${run.id}` : pr.html_url; + const output = { title: result.title, + // GitHub ignores details_url for checks created with GITHUB_TOKEN, so link the run here. + summary: (run ? `[See fork CI results](${details})\n\n` : '') + note + + `\n\nBase: ${pr.base.ref} @ ${pr.base.sha}${testedBase}`, + text: JSON.stringify(record) }; + // Re-read before publishing so a push or retarget during the API calls cannot get a stale success. + const { data: current } = await github.rest.pulls.get({ ...context.repo, pull_number: pr.number }); + if (current.head.sha !== pr.head.sha || current.base.sha !== pr.base.sha || current.base.ref !== pr.base.ref || current.state !== 'open') return; + const params = { ...context.repo, ...result, output, details_url: details }; + delete params.title; + delete params.note; + let oldRecord; + try { oldRecord = JSON.parse(latest?.output?.text || '{}'); } catch { oldRecord = {}; } + const sameRun = oldRecord.run_id === record.run_id && oldRecord.run_attempt === record.run_attempt && oldRecord.base_sha === record.base_sha && oldRecord.base_ref === record.base_ref; + if (!latest || (latest.status === 'completed' && (!sameRun || result.status !== 'completed'))) { + await github.rest.checks.create({ ...params, name: 'Fork CI', head_sha: pr.head.sha }); + } else if (!sameRun || latest.status !== result.status || latest.conclusion !== (result.conclusion || null) || latest.output?.summary !== output.summary) { + await github.rest.checks.update({ ...params, check_run_id: latest.id }); + } + } + async function visit(pr, work) { + processed++; + visited.add(pr.number); + if (pr.state !== 'open') return; + try { + await work(); + } catch (error) { + failures.push(pr.number); + core.warning(`Could not refresh #${pr.number}: ${error.message}`); + } + } + let processed = 0; + const visited = new Set(); + // Fork runs, including reruns of failed jobs, finish without an upstream event. Such PRs were + // updated recently, so probe those first with one call and fully refresh only unsettled checks. + const recent = manual ? [] : prs.filter(pr => now - Date.parse(pr.updated_at) < RECENT_MS) + .sort((a, b) => Date.parse(b.updated_at) - Date.parse(a.updated_at)); + let probed = 0; + for (const pr of recent) { + if (calls + 15 > budget) break; // One PR may need trees, runs, paginated jobs and a check write. + probed++; + await visit(pr, async () => { + const latest = await latestCheck(pr); + if (latest?.status === 'completed' && SETTLED.includes(latest.conclusion)) return; Review Comment: Fixed in [b3410720b3](https://github.com/apache/gravitino/commit/b3410720b3). No completed result is treated as final any more: for each recently updated PR with a completed check, the sweep lists the latest `fork-ci.yml` run in the fork and fully refreshes the PR when its run id or attempt differs from the one recorded on the check (the record now also stores the fork run, so a check mirroring a trusted run still notices a newer fork rerun). Only running checks and changed runs get a full refresh, which also stops failed/cancelled checks from being re-evaluated every sweep. Probes take two calls, so recent PRs rotate within two thirds of the sweep budget; the rest keeps rotating through older PRs. Added a regression for a successful check whose fork rerun fails. Verified in a sandbox (fork-of-fork standing in for apache): Fork CI went green, then re-running the fork run with a forced failure (attempt 2) turned the upstream check to `failure` on the next sweep, with `run_attempt: 2` recorded. Once records were rewritten, a sweep probed all 13 recent PRs in 37/60 API calls. ########## .github/workflows/build.yml: ########## @@ -375,21 +454,23 @@ jobs: - name: Generate Coverage Report id: coverage + if: steps.coverage-checkout.outputs.pull-request == '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.coverage-checkout.outputs.base-ref }}" \ + --head-sha "${{ github.event.pull_request.head.sha || github.sha }}" \ + --repo-url "${{ github.event.pull_request.head.repo.html_url || format('{0}/{1}', github.server_url, github.repository) }}" \ --min-overall 40 \ --min-changed 60 \ --output coverage-report.md + # coverage-comment.yml posts the report for pull_request runs in this repository. - name: Save PR number - if: steps.coverage.outputs.has_reports == 'true' + if: github.event_name == 'pull_request' && steps.coverage.outputs.has_reports == 'true' Review Comment: Agreed that the `pull_request`-based coverage comment no longer fires for Fork CI runs. This is listed under the known limitations in CONTRIBUTING (inspect the fork run's `build / coverage` log and `coverage-report` artifact), and we'd like to keep it out of this PR since it is not a merge gate. I'll follow up by having the trusted publisher post the report for the PR it mirrors (using the PR number it already trusts rather than `pr-number.txt`). `coverage-comment.yml` and the PR-number step stay for standalone copies that still build on `pull_request`. ########## .github/scripts/fork-ci-status.cjs: ########## @@ -0,0 +1,129 @@ +/* + * 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. + */ +'use strict'; +const UPSTREAM = { owner: 'apache', repo: 'gravitino' }; +const SUITES = ['build', 'backend', 'spark', 'flink', 'trino', 'iceberg_rest_trino', 'python', + 'frontend', 'access_control', 'idp_basic', 'mcp', 'maintenance', 'web_ui', 'charts', + 'contrib_catalog', 'asf_allowlist']; +const requiredName = (pr, baseSha = pr.base.sha) => `Required CI / ${pr.base.ref} / ${baseSha} / ${pr.head.sha}`; +const requiredAction = note => ({ status: 'completed', conclusion: 'action_required', + title: 'Fork CI needs attention', note }); + +function evaluate(pr, run, jobs, baseSha = pr.base.sha) { + const required = jobs.find(job => job.name === requiredName(pr, baseSha)); + if (run.status !== 'completed') return { status: 'in_progress', title: 'Fork CI is running' }; + if (run.conclusion !== 'success') return { status: 'completed', + conclusion: ['failure', 'cancelled', 'timed_out', 'action_required'].includes(run.conclusion) + ? run.conclusion : 'action_required', title: `Fork CI ${run.conclusion}` }; + if (!required || required.conclusion !== 'success') { + return requiredAction('The run did not finish Required CI for this exact head, base branch and base SHA. ' + + 'Update the branch and push again, or ask a committer to dispatch a trusted Fork CI run.'); + } + for (const suite of SUITES) { + const children = jobs.filter(job => job.name.startsWith(`${suite} / `)); + if (!children.length || !children.some(job => job.conclusion === 'success') || + children.some(job => !['success', 'skipped'].includes(job.conclusion))) { + return requiredAction(`The ${suite} caller did not report successful jobs. Open the run and verify its suites.`); + } + } + return { status: 'completed', conclusion: 'success', title: 'All required suites passed' }; +} + +function createService(github) { + const definitions = new Map(); + const trees = new Map(); + async function ciTree(owner, repo, sha) { + const key = `${owner}/${repo}@${sha}`; + if (!trees.has(key)) trees.set(key, (async () => { + const { data: root } = await github.rest.git.getTree({ owner, repo, tree_sha: sha }); + const entries = Object.fromEntries(root.tree.map(e => [e.path, e])); + const githubTree = entries['.github']; + const result = { policy: entries['.asf.yaml']?.sha || null }; + if (!githubTree) return result; + const { data: dirs } = await github.rest.git.getTree({ owner, repo, tree_sha: githubTree.sha }); + for (const name of ['workflows', 'actions', 'scripts']) result[name] = dirs.tree.find(e => e.path === name)?.sha || null; + return result; + })()); + return trees.get(key); + } + async function unchanged(pr) { + const key = `${pr.head.repo.full_name}@${pr.head.sha}:${pr.base.sha}`; + if (definitions.has(key)) return definitions.get(key); + const [base, head] = await Promise.all([ + ciTree(UPSTREAM.owner, UPSTREAM.repo, pr.base.sha), + ciTree(pr.head.repo.owner.login, pr.head.repo.name, pr.head.sha) + ]); + const same = JSON.stringify(base) === JSON.stringify(head); + definitions.set(key, same); + return same; + } + async function jobsFor(owner, repo, run) { + // Partial re-runs retain earlier successful jobs. Take each named job's newest + // execution, rather than requiring every caller to execute in the latest attempt. + const jobs = await github.paginate('GET /repos/{owner}/{repo}/actions/runs/{run_id}/jobs', + { owner, repo, run_id: run.id, filter: 'all', per_page: 100 }); + const latest = new Map(); + for (const job of jobs) { + if ((job.run_attempt || 1) > run.run_attempt) continue; + const previous = latest.get(job.name); + if (!previous || (job.run_attempt || 1) > (previous.run_attempt || 1) || + ((job.run_attempt || 1) === (previous.run_attempt || 1) && job.id > previous.id)) { + latest.set(job.name, job); + } + } + return [...latest.values()]; + } + async function testedBase(pr, jobs) { + // pr.base.sha is not refreshed by later pushes, so runs usually test a newer base commit. + // Accept that commit only if it is on the PR base branch. + const prefix = `Required CI / ${pr.base.ref} / `; + const suffix = ` / ${pr.head.sha}`; + const job = jobs.find(job => job.name.startsWith(prefix) && job.name.endsWith(suffix)); + const sha = job?.name.slice(prefix.length, -suffix.length); + if (!sha || sha === pr.base.sha || !/^[0-9a-f]{40}$/.test(sha)) return pr.base.sha; + const { data } = await github.rest.repos.compareCommitsWithBasehead({ ...UPSTREAM, + basehead: `${sha}...${pr.base.ref}`, per_page: 1 }); + return ['ahead', 'identical'].includes(data.status) ? sha : pr.base.sha; Review Comment: 1. Documented, in the code and in CONTRIBUTING: `pr.base.sha` is not refreshed by pushes, so requiring equality would turn nearly every push red; strict branch protection, not this check, is what blocks a merge whose branch is behind. The check summary shows the tested base when it differs. 2. Fixed in [b3410720b3](https://github.com/apache/gravitino/commit/b3410720b3). The publisher now reads the latest check immediately before writing, and skips the write when that check records a newer run id or attempt from the same repository, or a completed result for the same attempt while this publisher saw it running. Added a regression for both cases. ########## .github/scripts/fork-ci-status.cjs: ########## @@ -0,0 +1,129 @@ +/* + * 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. + */ +'use strict'; +const UPSTREAM = { owner: 'apache', repo: 'gravitino' }; +const SUITES = ['build', 'backend', 'spark', 'flink', 'trino', 'iceberg_rest_trino', 'python', + 'frontend', 'access_control', 'idp_basic', 'mcp', 'maintenance', 'web_ui', 'charts', + 'contrib_catalog', 'asf_allowlist']; +const requiredName = (pr, baseSha = pr.base.sha) => `Required CI / ${pr.base.ref} / ${baseSha} / ${pr.head.sha}`; +const requiredAction = note => ({ status: 'completed', conclusion: 'action_required', + title: 'Fork CI needs attention', note }); + +function evaluate(pr, run, jobs, baseSha = pr.base.sha) { + const required = jobs.find(job => job.name === requiredName(pr, baseSha)); + if (run.status !== 'completed') return { status: 'in_progress', title: 'Fork CI is running' }; + if (run.conclusion !== 'success') return { status: 'completed', + conclusion: ['failure', 'cancelled', 'timed_out', 'action_required'].includes(run.conclusion) + ? run.conclusion : 'action_required', title: `Fork CI ${run.conclusion}` }; + if (!required || required.conclusion !== 'success') { + return requiredAction('The run did not finish Required CI for this exact head, base branch and base SHA. ' + + 'Update the branch and push again, or ask a committer to dispatch a trusted Fork CI run.'); + } + for (const suite of SUITES) { + const children = jobs.filter(job => job.name.startsWith(`${suite} / `)); + if (!children.length || !children.some(job => job.conclusion === 'success') || + children.some(job => !['success', 'skipped'].includes(job.conclusion))) { + return requiredAction(`The ${suite} caller did not report successful jobs. Open the run and verify its suites.`); + } + } + return { status: 'completed', conclusion: 'success', title: 'All required suites passed' }; +} + +function createService(github) { + const definitions = new Map(); + const trees = new Map(); + async function ciTree(owner, repo, sha) { + const key = `${owner}/${repo}@${sha}`; + if (!trees.has(key)) trees.set(key, (async () => { + const { data: root } = await github.rest.git.getTree({ owner, repo, tree_sha: sha }); + const entries = Object.fromEntries(root.tree.map(e => [e.path, e])); + const githubTree = entries['.github']; + const result = { policy: entries['.asf.yaml']?.sha || null }; + if (!githubTree) return result; + const { data: dirs } = await github.rest.git.getTree({ owner, repo, tree_sha: githubTree.sha }); + for (const name of ['workflows', 'actions', 'scripts']) result[name] = dirs.tree.find(e => e.path === name)?.sha || null; + return result; + })()); + return trees.get(key); + } + async function unchanged(pr) { + const key = `${pr.head.repo.full_name}@${pr.head.sha}:${pr.base.sha}`; + if (definitions.has(key)) return definitions.get(key); + const [base, head] = await Promise.all([ + ciTree(UPSTREAM.owner, UPSTREAM.repo, pr.base.sha), + ciTree(pr.head.repo.owner.login, pr.head.repo.name, pr.head.sha) + ]); + const same = JSON.stringify(base) === JSON.stringify(head); + definitions.set(key, same); + return same; + } + async function jobsFor(owner, repo, run) { + // Partial re-runs retain earlier successful jobs. Take each named job's newest + // execution, rather than requiring every caller to execute in the latest attempt. + const jobs = await github.paginate('GET /repos/{owner}/{repo}/actions/runs/{run_id}/jobs', + { owner, repo, run_id: run.id, filter: 'all', per_page: 100 }); + const latest = new Map(); + for (const job of jobs) { + if ((job.run_attempt || 1) > run.run_attempt) continue; + const previous = latest.get(job.name); + if (!previous || (job.run_attempt || 1) > (previous.run_attempt || 1) || + ((job.run_attempt || 1) === (previous.run_attempt || 1) && job.id > previous.id)) { + latest.set(job.name, job); + } + } + return [...latest.values()]; + } + async function testedBase(pr, jobs) { + // pr.base.sha is not refreshed by later pushes, so runs usually test a newer base commit. + // Accept that commit only if it is on the PR base branch. + const prefix = `Required CI / ${pr.base.ref} / `; + const suffix = ` / ${pr.head.sha}`; + const job = jobs.find(job => job.name.startsWith(prefix) && job.name.endsWith(suffix)); + const sha = job?.name.slice(prefix.length, -suffix.length); + if (!sha || sha === pr.base.sha || !/^[0-9a-f]{40}$/.test(sha)) return pr.base.sha; + const { data } = await github.rest.repos.compareCommitsWithBasehead({ ...UPSTREAM, + basehead: `${sha}...${pr.base.ref}`, per_page: 1 }); + return ['ahead', 'identical'].includes(data.status) ? sha : pr.base.sha; + } + async function findRun(pr, trusted) { + const target = trusted ? UPSTREAM : { owner: pr.head.repo.owner.login, repo: pr.head.repo.name }; + const { data } = await github.rest.actions.listWorkflowRuns({ ...target, workflow_id: 'fork-ci.yml', Review Comment: Fixed in [b3410720b3](https://github.com/apache/gravitino/commit/b3410720b3): `findRun` uses `github.paginate(listWorkflowRuns)`, with a regression test. ########## .github/actions/checkout-and-sync/action.yml: ########## @@ -0,0 +1,139 @@ +# 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. + +inputs: + base-ref: + description: Fixed upstream branch selected by Fork CI. + required: false + default: '' + base-sha: + description: Fixed upstream commit selected by Fork CI. + required: false + default: '' + head-repository: + description: Repository containing the contributor commit. + required: false + default: '' + head-sha: + description: Fixed contributor commit to test. + required: false + default: '' + sync: + description: Whether to merge the contributor commit onto the fixed base. + required: false + default: '' + persist-credentials: + description: Whether to keep the token in the git config of the checkout. + required: false + default: 'true' Review Comment: Fixed in [b3410720b3](https://github.com/apache/gravitino/commit/b3410720b3): the composite now defaults `persist-credentials` to `false` (no caller needs credentials after checkout; the contributor commit is fetched from a public URL). On permissions: called reusable workflows do get the caller's workflow-level `permissions`. In a fork run (yuqi1129/gravitino run 39750424484), the called job `contrib_catalog / changes` logs `GITHUB_TOKEN Permissions: Actions: read, Contents: read, Metadata: read, PullRequests: read`, matching `fork-ci.yml`. Artifact upload uses the runtime token, not `GITHUB_TOKEN`, so `actions: write` is not needed; e.g. the PR head fork run qqqttt123/gravitino run 39752475623 uploaded `coverage-report`, `jacoco-build` and the core test evidence with `actions: read`. -- 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]
