andygrove commented on code in PR #7311: URL: https://github.com/apache/datafusion-comet/pull/7311#discussion_r4417587372
########## .github/workflows/fork_ci_status.yml: ########## @@ -0,0 +1,260 @@ +# 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. + +# Mirror a contributor's fork run (ci_fork.yml) onto its pull request as the informational `CI (fork)` check, +# after upstream PR CI completes and through scheduled polling for late runs and +# reruns. The check is feedback only: `Required Checks` does not depend on it, +# because a contributor controls the workflow that produced it, and the merge +# queue runs the queue tier on the merge result. +# +# Reads fork runs and writes upstream checks using reviewed default-branch code. +# No checkout, artifacts, or code from the pull request are used here. +# +# Adapted from apache/arrow-rs#11788 and apache/datafusion#25591. +# Test with Node.js 24: +# node --test dev/ci/test-fork-ci-status.cjs +# Tests mock GitHub APIs. End-to-end testing needs a fork pull request after this +# workflow is installed on the default branch. +name: Fork CI status + +on: + workflow_run: + workflows: [Comet CI] + types: [completed] + schedule: + - cron: '7,22,37,52 * * * *' + workflow_dispatch: + +permissions: + actions: read + checks: write + pull-requests: read Review Comment: Done in 876d95938a. The top level is `permissions: {}` and the `bridge` job asks for `checks: write` and `pull-requests: read`. `actions: read` was not needed: every `github.rest.actions.*` call targets the fork, and reading another repository's public runs and workflows does not use a scope of this repository's token. I kept `pull-requests: read` because the job lists this repository's open pull requests, which is the one read it does here. I have not run the bridge without `actions: read`, since it cannot run until it is on the default branch, so the first fork pull request after merge is the real test. `check-ci-config.py` now pins the top level to `{}` and the job to exactly those two scopes, and fails if `actions: read` or a top-level scope comes back. ########## dev/ci/fork-push-changed-files.sh: ########## @@ -0,0 +1,56 @@ +#!/usr/bin/env bash +# +# 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. + +# Prints the files a push to a branch of a contributor's fork changed, as the +# pull request from that branch would show them: relative to upstream main. +# +# There is no pull request to diff against on a push, and the fork's own default +# branch is the wrong base. A fork that has not synced is behind upstream, so a +# branch based on newer upstream code that reverts one of its files matches the +# fork's old copy, and a diff against it omits the file. Fetching upstream main +# gives the diff the pull request will have. The fetch drops the credentials +# actions/checkout leaves in the git config: they belong to the fork, and the +# upstream repository is public. +# +# Prints every tracked file when there is nothing to go on: the fetch fails, there +# is no merge base, or the diff is empty. An empty diff means the branch adds +# nothing to upstream main, such as `git push fork main:my-fix` before upstream +# has the commits, and selecting no suites would report a green run for a change +# nobody built. Review Comment: You're right, and the example was wrong: with upstream as the base, `main:my-fix` lists the commits the fork's `main` has and upstream lacks, as the test asserts. The case I was guarding against was the old base, the fork's own default branch. The script now prints nothing for an empty diff with a merge base, since the pull request will show no change, and lists every file only when the fetch fails or there is no merge base. The test for the empty case now expects nothing, and the README and guide text no longer repeat the example. ########## .github/workflows/fork_ci_status.yml: ########## @@ -0,0 +1,260 @@ +# 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. + +# Mirror a contributor's fork run (ci_fork.yml) onto its pull request as the informational `CI (fork)` check, +# after upstream PR CI completes and through scheduled polling for late runs and +# reruns. The check is feedback only: `Required Checks` does not depend on it, +# because a contributor controls the workflow that produced it, and the merge +# queue runs the queue tier on the merge result. +# +# Reads fork runs and writes upstream checks using reviewed default-branch code. +# No checkout, artifacts, or code from the pull request are used here. +# +# Adapted from apache/arrow-rs#11788 and apache/datafusion#25591. +# Test with Node.js 24: +# node --test dev/ci/test-fork-ci-status.cjs +# Tests mock GitHub APIs. End-to-end testing needs a fork pull request after this +# workflow is installed on the default branch. +name: Fork CI status + +on: + workflow_run: + workflows: [Comet CI] + types: [completed] + schedule: + - cron: '7,22,37,52 * * * *' + workflow_dispatch: + +permissions: + actions: read + checks: write + pull-requests: read + +# Like DataFusion, group notifications separately from scheduled/manual sweeps. +# workflow_run identifies the fork and branch rather than the PR number. +concurrency: + group: ${{ github.workflow }}-${{ github.event.workflow_run.head_repository.full_name || 'update' }}-${{ github.event.workflow_run.head_branch }} + cancel-in-progress: ${{ github.event_name == 'workflow_run' }} + +jobs: + bridge: + # A workflow_run for a pull request from a branch of this repository has no + # fork run to mirror, so it never starts a job. + if: >- + github.repository == 'apache/datafusion-comet' + && (github.event_name != 'workflow_run' + || (github.event.workflow_run.event == 'pull_request' + && github.event.workflow_run.head_repository.full_name != github.repository)) + name: Mirror the fork run + runs-on: ubuntu-slim + timeout-minutes: 10 + steps: + - name: Create or update the check + uses: actions/github-script@v9 + with: + script: | + // Adapted from apache/datafusion#25591. Fork results are informational: + // contributors control their workflows; only upstream merge-queue CI gates merging. + const CHECK = 'CI (fork)'; + const WORKFLOW = 'ci_fork.yml'; + const upstream = { owner: context.repo.owner, repo: context.repo.repo }; + const repository = upstream.owner + '/' + upstream.repo; + const GUIDE = 'https://github.com/apache/datafusion-comet/blob/main/docs/source/contributor-guide/ci.md#pull-requests-from-forks'; + const SWEEP_SIZE = 25; + const listing = { ...upstream, state: 'open', base: 'main', sort: 'updated', direction: 'desc' }; + const isForkPr = (pr) => pr.state === 'open' && pr.head.repo && + pr.head.repo.full_name.toLowerCase() !== repository.toLowerCase() && pr.base.ref === 'main'; + const externalId = (pr) => 'comet-fork-ci:' + pr.number; + const conclusions = new Set([ + 'action_required', 'cancelled', 'failure', 'neutral', 'skipped', 'success', 'timed_out', + ]); + const UPDATE_NOTE = + 'This check is informational: the contributor controls the fork workflow. ' + + 'The upstream merge queue runs the required CI checks on the proposed merge result. ' + + 'Active or missing runs on recently updated PRs are polled every 15 minutes; ' + + 'a rotating sweep also discovers reruns on older PRs. Scheduled updates may be delayed.'; + + // An absent workflow/run is expected on a fork with Actions disabled. Other + // API errors must fail the bridge rather than falsely reporting missing CI. + async function optional(request) { + try { + return (await request()).data; + } catch (error) { + if (error.status === 404) return null; + throw error; + } + } + + async function syncCheck(pr, includeCompleted = true) { + if (!isForkPr(pr)) return; + const fork = { owner: pr.head.repo.owner.login, repo: pr.head.repo.name }; + const sha = pr.head.sha; + const checks = await github.paginate(github.rest.checks.listForRef, { + ...upstream, ref: sha, check_name: CHECK, per_page: 100, + }); + const check = checks.find((item) => + item.app?.slug === 'github-actions' && item.external_id === externalId(pr)); + let previous = {}; + try { previous = JSON.parse(check?.output?.text || '{}'); } catch {} + // A missing run must keep being polled, even though its check is completed. + if (!includeCompleted && check?.status === 'completed' && previous.run_id) return; + + // A fork's default branch is never tested: ci_fork.yml skips it, and a + // push there is a sync, which runs nothing. Looking for a run would + // find none, or worse, one that tested nothing and passed. + const fromMain = pr.head.ref === pr.head.repo.default_branch; + const data = fromMain ? null : await optional(() => github.rest.actions.listWorkflowRuns({ + ...fork, workflow_id: WORKFLOW, head_sha: sha, branch: pr.head.ref, per_page: 100, + })); + // Ignore PR merge-ref runs. The fork workflow on push/manual events + // tests the branch head; contributors still control its code. + const run = (data?.workflow_runs || []).filter((item) => + item.head_sha === sha && item.head_branch === pr.head.ref && + ['push', 'workflow_dispatch'].includes(item.event) Review Comment: Agreed, `ci_fork.yml` has no other trigger, so the filter is now `item.event === 'push'` with the comment updated. I removed the manual-run case that expected a `workflow_dispatch` run to win, and the other now asserts that a dispatch run is ignored. I checked that test fails if the `workflow_dispatch` match comes back. ########## dev/ci/compute-changes.py: ########## @@ -660,7 +708,7 @@ def event_allows(job, event): if gates: if not any(label in event.get("labels", []) for label in gates): return False - elif "pr" not in tiers: + elif "pr" not in tiers or is_fork_pull_request(event): Review Comment: Right, that is a behavior change and the description was wrong to call it unchanged. For a fork pull request with `run-all-spark-profiles`, only the four non-default profiles run here now, and the fork's run covers the default profile and the lints, so between them every profile runs; without a fork run the label no longer brings the lints or the 4.1 row forward. I kept that behavior, since running the default profile here as well would duplicate the fork's run, and made it explicit: `ci.yml`'s profile table has a row for a fork pull request with and without the label, `POLICY_CASES` pins `fork_pr(labels=["run-all-spark-profiles"])` on `synchronize` to `build_linux_all_profiles` alone, the guide says what the label covers on a fork pull request, and the description is corrected. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
