shahar1 commented on PR #74889: URL: https://github.com/apache/airflow/pull/74889#issuecomment-6254129600
Thanks for the thorough review. All points stand, and the new commit addresses them while staying on Flex. The reasoning behind that choice, since you asked for it to be checked: **Why Flex, hardened, rather than Fleet.** Fleet would need its own org-level GitHub App (`organization_self_hosted_runners: write`, installed on the `apache` org), an org runner group, and a Terraform-only deployment the current CloudFormation stack cannot provide. The gate it buys, "selected workflows" on the runner group, is ref-based: GitHub pins non-reusable workflows to a branch, so `refs/pull/N/merge` runs never qualify. Fleet would cover canaries and pushes only and drop the maintainer-PR half of this PR, as you noted. The only PR path under Fleet is reusable workflows pinned to `@refs/heads/main`, which would take CI workflow changes out of PR testing. Not worth it for us. Flex on a hardened stack is also what ASF Infra already runs for `pull_request` jobs of public repositories today (apache/datafusion, apache/arrow-nanoarrow, apache/infrastructure-asfquart all request RunsOn labels on PR runs), so the model is not new to Infra. I will raise two things with them before this merges: the repository setting "Require approval for all external contributors", and confirmation of the stack posture (instance role limited to the cache bucket, `EnableStickyDiskIsolation`, `RunnerMaxRuntime`, no SSH, runner-to-runner isolation, EC2 vCPU quotas as the hard cost ceiling, since RunsOn has no instance cap and `AppBudgetDailyUsd` only alerts). **What the new commit (`e8d62bbb33`) changes** 1. Routing is documented as a preference. Your docstring went in as suggested, and `04_selective_checks.md` now lists what is enforced outside the repository instead of claiming selective checks keep fork code off RunsOn. 2. The `use self-hosted runners` label counts only while a committer's latest review of `pull_request.head.sha` is an approval and no committer's request for changes stands (reviews API; the veto follows GitHub's merge blocking across pushes until that committer approves or the request is dismissed). The approval replaces the "who added the label" check rather than adding to it: the approval is the vouch, the label only the opt-in. Dropping the label on `synchronize` is not possible here: fork PR runs have a read-only token and the repository bans `pull_request_target`. 3. `vars.USE_PUBLIC_RUNNERS` is read by selective checks on both platforms and moves every run, pushes included, back to public runners. Write access is enough to set it. 4. `move_docker_to_mnt.sh` leaves docker alone on any self-hosted runner, as suggested. 5. `ci-image-build.yml` runs `make_mnt_writeable.sh` and `move_docker_to_mnt.sh` from the `workflow-version` checkout, which now also holds `scripts/ci`. 6. The S3 stash is streamed while hashed (fifo-fed `sha256sum`, `tee`, `aws s3 cp -` to a temporary key, server-side copy to the digest key). The image tarball no longer touches the runner's disk. 7. The S3 restore fails when a restored entry is outside `path`. 8. The cost summary lives in its own composite action, used by the breeze action and directly by the 13 RunsOn-capable jobs that do not install Breeze. 9. A comment next to the image restores records why `only-current-branch` must stay. The mount cache keeps going to GitHub as an artifact: PRs on public runners read it from `main`, so S3 cannot hold it. --- Drafted-by: Claude Code (Fable 5.1) (no human review before posting) -- 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]
