ci: fall back to ubuntu-latest for fork PRs#49
Merged
Conversation
GitHub Actions disables self-hosted runners for fork PRs by security
policy — external contributors' PRs would otherwise hang forever on
'waiting for runner'. Adding a conditional runs-on resolves to:
- same-repo PRs and push: cachekit-lean / cachekit (existing behavior,
cheap on the self-hosted infra)
- fork PRs: ubuntu-latest (GitHub-hosted, available to forks)
Applied to all 8 jobs in ci.yml. Also removes the misleading
"needs DinD" comment on test-integration — it uses GHA service
containers, which work natively on ubuntu-latest. The original
cachekit (full self-hosted) was for compute, not Docker.
No new permissions required; the change is a pure scheduler hint.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR makes CI jobs fork-aware by using ChangesFork-aware CI runner selection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 66: The runs-on expression uses the fallback label 'cachekit' which
differs from other jobs; update the conditional expression that sets runs-on
(the string literal currently 'cachekit') to use the same fallback label
'cachekit-lean' so push/same-repo PRs schedule consistently with other jobs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| # Fork PRs fall back to ubuntu-latest (GHA disables self-hosted runners | ||
| # for forks by security policy). The redis service container below works | ||
| # on either runner — the original DinD note was misleading. | ||
| runs-on: ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' || 'cachekit' }} |
There was a problem hiding this comment.
Use the same self-hosted fallback label as other jobs.
Line 66 uses 'cachekit' while the rest of the workflow uses 'cachekit-lean'. This inconsistency can break scheduling for test-integration on push/same-repo PRs if cachekit is not an available label.
Proposed fix
- runs-on: ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' || 'cachekit' }}
+ runs-on: ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' || 'cachekit-lean' }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 66, The runs-on expression uses the
fallback label 'cachekit' which differs from other jobs; update the conditional
expression that sets runs-on (the string literal currently 'cachekit') to use
the same fallback label 'cachekit-lean' so push/same-repo PRs schedule
consistently with other jobs.
The previous comment claimed the DinD note on the original runner choice
was "misleading" — it wasn't. I was wrong.
Verified against ARC runner specs (kubectl get autoscalingrunnerset):
- cachekit: DOCKER_HOST set, TLS certs mounted, full DinD
- cachekit-lean: no Docker env, runs as user 1001, service account
literally named cachekit-lean-gha-rs-no-permission
The redis service container needs Docker on the host. cachekit-lean
can't host it; only cachekit can. The runner-name difference between
test-integration and every other job is intentional and load-bearing —
flattening to cachekit-lean would break integration tests on internal
runs.
Comment now reflects that constraint accurately so future readers
(including reviewers) don't repeat the suggestion.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GitHub Actions disables self-hosted runners for fork PRs by security policy. With every CI job currently pinned to
cachekit-lean/cachekit, an external contributor's PR would sit inwaiting for runnerforever — silent friction that kills OSS adoption.This PR adds a conditional
runs-onto all 8 jobs inci.yml:Resolution table:
head.repo.forkpush: maincachekit-leanfalsecachekit-leantrueubuntu-latestSo internal-author PRs and pushes keep the existing self-hosted scheduling (cheap on org infra), while fork PRs land on GitHub-hosted runners and actually run.
The
test-integrationjob also picks up the same conditional (falls back toubuntu-latest). Bonus: removes a misleading comment — the job'sservices.redisis a GHA service container, which works natively onubuntu-latest. It doesn't require DinD; the originalcachekitrunner choice was for compute, not Docker.Why this matters now
Repo is in OSS-prep. As soon as you flip
cachekit-tspublic, the first fork PR will expose this. Better to land the fix before users hit it.Why not just use ubuntu-latest everywhere?
That would shift every internal push to GitHub-hosted minutes (~$0.008/min × 8 jobs × every CI run). The self-hosted runners exist specifically to avoid that bill — keeping them for internal traffic and only paying for public runners when actually needed is the cost-rational middle ground.
Test plan
actionlintclean (run locally via prek)cachekit-leansince you have repo access)ubuntu-latestfallback (manual verification once public)Out of scope
main(already applied directly via gh API)Summary by CodeRabbit