ci(bazel): glob-gate root on main too (drop the every-merge force)#327
Open
balajinvda wants to merge 1 commit into
Open
ci(bazel): glob-gate root on main too (drop the every-merge force)#327balajinvda wants to merge 1 commit into
balajinvda wants to merge 1 commit into
Conversation
#311 forced the root row on every push to main to keep the cache warm. That was a workaround for the wrong hypothesis: root was cold because it never uploaded (its .bazelrc --config=remote pinned --remote_upload_local_results=false), not because it ran too rarely. With the upload fix in place and the cache rings not evicting, root only needs to run (and upload) when a ROOT_GLOB file actually changes -- a later root PR reads the closure warmed by the last root-touching merge. Forcing root on every merge just spun up a ~7k-target job for docs- and helm-only merges that warm nothing new. Make root purely glob-gated on merges as it already is on PRs; keep the main-push upload (CACHE_UPLOAD) so warming still happens whenever root runs. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Bazel workflow’s matrix detection no longer passes ChangesBazel matrix detection
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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/bazel.yml:
- Around line 152-162: Expand the root-gate matching around ROOT_GLOBS to
include WORKSPACE, WORKSPACE.bazel, go.work, go.work.bazel, and root-level *.bzl
changes, keeping behavior consistent with the full-build triggers in the
target-selection logic. Use a matcher that supports the literal root-level *.bzl
pattern, since the current fixed-prefix awk check cannot interpret it;
preferably reuse a canonical root-global matcher if one exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5e83c395-8a76-49be-938b-fa5ca669add3
📒 Files selected for processing (1)
.github/workflows/bazel.yml
Comment on lines
+152
to
+162
| # Root is glob-gated on PRs and merges alike: it runs only when a | ||
| # ROOT_GLOB file changed (a docs- or helm-only change never triggers | ||
| # it). When it does run on a main push it uploads (CACHE_UPLOAD), | ||
| # warming the cache; the rings do not evict, so a later root PR reads | ||
| # that warm closure. | ||
| for g in "${ROOT_GLOBS[@]}"; do | ||
| # Fixed-string, start-of-line prefix test via awk index()==1: | ||
| # avoids treating the dots in .bazelrc/MODULE.bazel as regex | ||
| # wildcards and avoids any shell glob expansion of the token. | ||
| if printf '%s\n' "$changed" | awk -v p="$g" 'index($0, p) == 1 { f = 1 } END { exit !f }'; then hit=true; break; fi | ||
| done |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include all root-wide Bazel inputs in the root gate.
When run_all=false, this is the only path selecting the root row, but ROOT_GLOBS does not cover WORKSPACE, WORKSPACE.bazel, go.work, go.work.bazel, or root-level *.bzl changes. The later target-selection logic treats these as full-build triggers (Lines [333]-[339]), so such changes can select no matrix rows and pass verification without running Bazel. Expand the gate or share a canonical root-global matcher; note that the current fixed-prefix awk check cannot match a literal *.bzl pattern.
🤖 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/bazel.yml around lines 152 - 162, Expand the root-gate
matching around ROOT_GLOBS to include WORKSPACE, WORKSPACE.bazel, go.work,
go.work.bazel, and root-level *.bzl changes, keeping behavior consistent with
the full-build triggers in the target-selection logic. Use a matcher that
supports the literal root-level *.bzl pattern, since the current fixed-prefix
awk check cannot interpret it; preferably reuse a canonical root-global matcher
if one exists.
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.
Why
#311 forced the
rootrow on every push to main to keep the remote cache warm. That was a workaround for the wrong hypothesis. The real reason root was always cold was that it never uploaded: root's.bazelrc--config=remotepinned--remote_upload_local_results=false, so even when root ran on a root-change merge it warmed nothing. That is fixed (forceupload=trueon main +.bazelrcconsistency).With the upload fix in place and the cache rings not evicting, root only needs to run when a
ROOT_GLOBfile changes: a later root PR reads the closure warmed by the last root-touching merge. Forcing root on every merge just spun up the ~7k-target build for docs- and helm-only merges that warm nothing new.What changed
is_main_pushforce in the detect job. Root is now purely glob-gated on merges, exactly as it already is on PRs — a docs- or helm-only change never triggers root.CACHE_UPLOAD), so root still warms the cache whenever it does run.REFenv.Testing
YAML parses; the detect
run:block passesbash -n. A docs-only merge will no longer schedule the root row.Issues
Relates to #283
References
Refines the warming approach from #311.
Dependencies
None
Summary by CodeRabbit