jerryshao commented on code in PR #13049: URL: https://github.com/apache/gravitino/pull/13049#discussion_r4346696605
########## .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: The schedule sweep treats `success` and `action_required` as final for six hours (`update-fork-ci.cjs:18-20` and here) and then marks the PR visited, so the rotating pass skips it too. Fork reruns never emit an upstream `workflow_run`, so a failed rerun leaves Fork CI green, and `action_required` stays stuck after the contributor reruns. Since comments keep `updated_at` fresh, this can last indefinitely. Please re-fetch the latest `fork-ci.yml` run for recent PRs even when the check is settled, and publish when the run id or run attempt changes. ########## .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: nit: `persist-credentials` still defaults to `true` here, and most suite callers don't pass `false` (e.g. `web-ui-tests.yml:67` also checks out without disabling it). Please default the composite to `false`. Also note that workflow-level `permissions` in `fork-ci.yml` apply only to its own jobs; called reusable workflows get the repository default token unless the calling job sets `jobs.<id>.permissions`. If you lock those down, remember `actions: write` may be needed for artifact upload. ########## .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: nit: `findRun` calls `listWorkflowRuns` with `per_page: 100` and does not paginate. Please use `github.paginate` here, as `resolve-fork-ci.cjs` does for branches, so an older run cannot win once a SHA has more than one page of runs. ########## .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: `coverage-comment.yml` (lines 4-16, not in this diff) still listens for a completed `build` workflow whose event is `pull_request`. `build.yml` is now `workflow_call` only, so that never fires, and `pr-number.txt` is saved only when `github.event_name == 'pull_request'` (here), which Fork CI never is (`push` / `workflow_dispatch`). As a result, coverage comments will not run anymore. Please publish the report from a trusted default-branch workflow (e.g. the publisher, or a `workflow_run` on `Fork CI`) and always record the PR number for synced fork runs. ########## .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: `testedBase` publishes success when the tested base is only an ancestor of the PR base (`ahead`). `strict: true` blocks the merge, so that's OK, but the check itself does not go red; please either fail it when the mirrored base differs from `pr.base.sha` or document that `strict: true` is the invalidation. Separately, both the per-SHA signal and the sweep can write the check (`update-fork-ci.cjs:76-87`), and the later write wins even if it observed an older attempt. Combined with the 6-hour settled skip, a stale success can become sticky. Please only publish when `(run_id, run_attempt)` is newer than what is already recorded on the SHA. -- 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]
