ci(bazel): run root tests, quarantine flaky go-lib, add missing service rows#311
Merged
Conversation
The root matrix row ran with tests_skip: true, so no root tests ran in CI (nvcf-cli, the Java example, and all go-lib packages). The skip existed only because two go-lib areas fail in the GitHub Actions environment: pkg/core (core_test goroutine-leak detector) and pkg/icms-translate (env-flaky). Enable the row's tests and quarantine just those two packages via .github/bazel-root-test-quarantine.txt, whose negative patterns are subtracted from the resolved test set (works for both the full //... set and a narrowed affected set). The file lives at the repo root, so the subtraction applies only to the root row. nvcf-go owns the test-layer fix; removing an entry restores its coverage (#284). Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Bazel workflow now runs root tests with targeted quarantines, resolves PR diffs from the base commit, uses explicit cache controls, and adds reusable Starlark rules for packaging Go binaries as multi-architecture OCI images. ChangesBazel CI test selection
Go OCI image rules
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BazelCI as Bazel CI workflow
participant Quarantine as Quarantine file
participant Bazel as Bazel test
BazelCI->>Quarantine: Read negative selectors
Quarantine-->>BazelCI: Return quarantined patterns
BazelCI->>Bazel: Run root tests with negated selectors
Bazel-->>BazelCI: Return filtered test results
sequenceDiagram
participant GoBinary
participant GoOCI as go_oci_image
participant OCIComposition as create_oci_image
participant OCIIndex as oci_image_index
GoBinary->>GoOCI: Provide binary target
GoOCI->>OCIComposition: Create binary tar layer and image targets
OCIComposition->>OCIIndex: Aggregate platform images
Possibly related issues
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 431-442: Update the root-change detection configuration,
specifically ROOT_GLOBS, to include .github/bazel-root-test-quarantine.txt so
changes to that file select the root CI row. Preserve the existing detection
behavior for all other root patterns.
🪄 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: e29dc3f4-01e8-44e7-8210-d91fea754e25
📒 Files selected for processing (2)
.github/bazel-root-test-quarantine.txt.github/workflows/bazel.yml
The first enabled root test run (78 tests, 76 pass) showed the real failing set differs from the old blanket-skip comment: - //src/libraries/go/lib:gazelle_test fails 3/3 (committed BUILD files are stale vs the pinned Gazelle; a codegen fix, not flakiness). - //src/libraries/go/lib/cmd/icms-translate:icms-translate_test fails 3/3. - pkg/icms-translate uses process-global os.Setenv in tests (env-flaky). pkg/nvkit/auth:auth_test is genuinely flaky (timing) but recovers under --flaky_test_attempts=3, so it is not quarantined. pkg/core is not quarantined: the old goroutine-leak claim is unsupported (no leak detector in its tests), so its tests now run and stand or fall on their own. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
byoo-otel-collector, nvcf-unbound, and the four workers (worker-init, worker-llm-credentials, worker-task, worker-utils) each have their own MODULE.bazel and a public OCI base (nvcr.io/nvidia/distroless/go, digest anonymously pullable), but were never added as matrix rows. So a change touching only one of them (e.g. #219 for byoo, which even edits cli_test.go) matched no row, produced an empty matrix, and skipped Bazel entirely with a green check. Add them so their build and tests run on PRs and main. nvsnap remains the only intentional exclusion (private, non-public OCI base; see the detect comment). Rows start with tests enabled; any that fail tests in the GHA environment will be flipped to build-only with a tracking issue, per the documented policy. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
balajinvda
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/bazel.yml (1)
443-446: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle an empty quarantine file without failing CI.
With
set -euo pipefail,grepreturns status 1 when the file contains only comments or blank lines, causing the pipeline at Line 446 to fail. The contract says an empty file should run the entire root module.Suggested fix
- grep -E '^[[:space:]]*-//' "$qfile" | sed -E 's/^[[:space:]]+//' >> "$ttfile" + awk '/^[[:space:]]*-\/\// { sub(/^[[:space:]]+/, ""); print }' "$qfile" >> "$ttfile"🤖 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 443 - 446, Update the quarantine-file parsing around qfile, n, and the grep/sed pipeline so files containing only comments or blank lines do not fail under set -euo pipefail. Treat such a file as having no quarantined targets and preserve the existing behavior that runs the entire root module.
🤖 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.
Outside diff comments:
In @.github/workflows/bazel.yml:
- Around line 443-446: Update the quarantine-file parsing around qfile, n, and
the grep/sed pipeline so files containing only comments or blank lines do not
fail under set -euo pipefail. Treat such a file as having no quarantined targets
and preserve the existing behavior that runs the entire root module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 03e61c6b-fe29-450a-9221-d996fb293df7
📒 Files selected for processing (1)
.github/workflows/bazel.yml
The module's cmd/BUILD.bazel loads //rules/oci:defs.bzl, but its rules/oci
directory shipped with only BUILD.bazel -- defs.bzl, transition.bzl, and the
private/{common,go}.bzl helpers were missing, so bazel build //... failed with
'cannot load //rules/oci/private:go.bzl: no such file'. This never surfaced
because the module was not in the CI matrix. Copy the complete, byte-identical
rules/oci tree the sibling workers already carry.
Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three matrix fixes surfaced by enabling root tests and adding service rows: - Root never warmed the remote cache: root's .bazelrc pins --config=remote, which sets --remote_upload_local_results=false; the workflow only overrode it to false for PRs and relied on the default (true) on main, so root's warm main-push builds silently never uploaded. Force the flag true when uploading. - Affected-target narrowing never fired: the Determine step resolved the base as origin/<branch>, which is not created inside the container checkout, so it always fell back to //.... Diff against the PR base SHA (present via fetch-depth: 0) instead. - byoo-otel-collector build needs a system go for its otelcol-contrib genrule, which ghcr bazel-ci:0.8.0 lacks. Bump to 0.11.0 (Go baked in, matches GitLab). Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 `@src/compute-plane-services/worker-llm-credentials/rules/oci/private/go.bzl`:
- Around line 57-84: Add repo-native coverage for the go_oci_image macro and its
_go_oci_image_impl outputs. Instantiate the macro with representative binary,
base, entrypoint, tags, and registry values, then verify generated targets for
both Linux platforms, the multi-architecture index, tar/load output, and the
optional push target; also cover the registry-omitted case to confirm push is
not generated.
🪄 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: 5a085e5f-5d48-496e-aa48-b2b408606c79
📒 Files selected for processing (5)
.github/workflows/bazel.ymlsrc/compute-plane-services/worker-llm-credentials/rules/oci/defs.bzlsrc/compute-plane-services/worker-llm-credentials/rules/oci/private/common.bzlsrc/compute-plane-services/worker-llm-credentials/rules/oci/private/go.bzlsrc/compute-plane-services/worker-llm-credentials/rules/oci/transition.bzl
…change - .bazelrc: drop the unconditional 'build --config=remote'. It pinned every build to the internal read-only nvcfbarn cache (unreachable from GitHub) and forced --remote_upload_local_results=false, which is why root never warmed. The per-service modules bake in no cache config; the caller (CI or a dev's ~/.bazelrc.user) supplies the endpoint and upload policy. Root now matches. The remote / remote-write configs stay defined for explicit opt-in. - Add .github/bazel-root-test-quarantine.txt to ROOT_GLOBS so a PR that edits only the quarantine list still runs the root row (CodeRabbit). Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 @.bazelrc:
- Around line 46-59: Reconcile the remote-cache policy documentation across the
`.bazelrc` comments: update the stale references around the `remote` and
`remote-write` configurations so they consistently reflect that caching is
opt-in, `remote` is read-only, and `remote-write` is permitted for the
documented internal/on-VPN workflows rather than being CI-only. Make the policy
and upload-mode guidance consistent throughout the file.
🪄 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: 34333b2c-affd-4fda-883e-f396b81a99ad
📒 Files selected for processing (2)
.bazelrc.github/workflows/bazel.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/bazel.yml
byoo-otel-collector's otelcol-contrib genrule shells out to a system go (rules_go's hermetic toolchain is rejected for that collector builder). The image now ships Go 1.25.6 (nvcf/bazel-ci-templates!4 cut the 0.12.x line with Go), so the byoo row builds. All rows move to 0.12.2 together. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
0.12.2 was a mistaken tag (not in ghcr) and broke every row on image-pull. The real version is 0.12.0 from nvcf/bazel-ci-templates!4, now mirrored to ghcr.io/nvidia/nvcf/bazel-ci:0.12.0. It ships Go 1.25.6 so byoo-otel-collector's otelcol-contrib genrule builds. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
With Go now in the CI image, the otelcol-contrib genrule gets past 'cannot find go' but fails with 'error obtaining VCS status: exit status 128': go build runs in Bazel's sandbox where there is no .git, so default VCS stamping errors. Add -buildvcs=false to both arch build commands. Builds on GitLab where .git is present were unaffected; the flag is harmless there. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kristinapathak
approved these changes
Jul 21, 2026
This was referenced Jul 21, 2026
balajinvda
added a commit
that referenced
this pull request
Jul 21, 2026
The test was written for nv-boot-parent as a standalone workspace root, so it
resolved generate_notice.py and its metadata at ${workspace}/tools/bazel/... and
checked ${workspace}/NOTICE. In the monorepo the workspace root is the repo
root, so those files live under src/libraries/java/nv-boot-parent/tools/bazel and
the import failed (No module named 'generate_notice'); it also checked the
aggregate root NOTICE (Go+Rust+Java) with only nv-boot's Java roots, which can
never match.
Point the script at the subtree prefix and at nv-boot-parent's own NOTICE, and
swap the //:NOTICE data dep for //src/libraries/java/nv-boot-parent:NOTICE. The
Maven lock stays the single root //:maven_install.json (where the Java dep
versions live). This never ran before #311 enabled root tests.
Co-authored-by: Balaji Ganesan <bganesan@nvidia.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
pdmack
pushed a commit
to pdmack/nvcf
that referenced
this pull request
Jul 21, 2026
…DIA#324) The test was written for nv-boot-parent as a standalone workspace root, so it resolved generate_notice.py and its metadata at ${workspace}/tools/bazel/... and checked ${workspace}/NOTICE. In the monorepo the workspace root is the repo root, so those files live under src/libraries/java/nv-boot-parent/tools/bazel and the import failed (No module named 'generate_notice'); it also checked the aggregate root NOTICE (Go+Rust+Java) with only nv-boot's Java roots, which can never match. Point the script at the subtree prefix and at nv-boot-parent's own NOTICE, and swap the //:NOTICE data dep for //src/libraries/java/nv-boot-parent:NOTICE. The Maven lock stays the single root //:maven_install.json (where the Java dep versions live). This never ran before NVIDIA#311 enabled root tests. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
2 participants
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
The root Bazel matrix row ran with
tests_skip: true, so no root tests ran inCI at all: not
nvcf-cli, not the Java example, not anysrc/libraries/go/libpackage. The blanket skip existed only because two go-lib areas fail in the
GitHub Actions environment:
//src/libraries/go/lib/pkg/core/...(core_testgoroutine-leak detector)//src/libraries/go/lib/pkg/icms-translate/...(env-flaky)What changed
Flip the root row to run tests (
tests_skip: false).Quarantine just the two known-flaky packages via
.github/bazel-root-test-quarantine.txt. Its negative Bazel patterns areappended after the positive test patterns, so Bazel subtracts them from the
resolved set. This works for both the full
//...set and a narrowedaffected-target set, and the file lives at the repo root so the subtraction
applies only to the root row.
Do not edit the native go-lib BUILD files (they are source, not GitHub-only
mirror files); the quarantine is a CI-side list the nvcf-go team clears as
they fix each package.
Add the 6 buildable services that were missing from the
detectmatrix:byoo-otel-collector,nvcf-unbound, and the four workers (worker-init,worker-llm-credentials,worker-task,worker-utils). Each has its ownMODULE.bazeland a public OCI base, but was never a matrix row, so a changetouching only one of them (surfaced by fix(byoo-otel-collector): log otelcol-contrib exit error on shutdown #219 for byoo) skipped Bazel entirely
with a green check.
nvsnapstays excluded (private OCI base, documented).Additional matrix fixes surfaced by the new coverage:
worker-llm-credentials: completed itsrules/ocicopy (was missingdefs.bzl/transition.bzl/private/*), which broke its build.byoo-otel-collector: bumped the workflow imagebazel-ci:0.8.0->0.11.0(Go baked in) for its
otelcol-contribgenrule.--remote_upload_local_results=trueon mainso root actually uploads (its
.bazelrc--config=remotehad pinned it off).unresolvable
origin/<branch>ref inside the container.Customer Release Notes
Not customer visible (CI only).
Plan Summary
Not applicable.
Usage
Not applicable.
Testing
YAML parses; every
run:block passesbash -n; the quarantine subtraction wasverified to produce
//...plus the two negative patterns. This PR itself editsa non-Go file, so its root job full-builds and runs the whole root test suite
minus the quarantine: a green root job confirms the quarantine set is sufficient;
a red one names any additional failing package to add.
Notes
This is the test-coverage counterpart to #283's build-speed work. The two
quarantined packages are owned by nvcf-go for a test-layer fix (goroutine-leak
cleanup and the environment dependency); production code must not change to make
them pass. When the file is empty, the whole root module is under test.
Issues
Relates to #284
Related Merge Requests/Pull Requests
Follows #280
Dependencies
None
Summary by CodeRabbit
Chores
New Features
Documentation