build(go): declare the Go toolchain once and derive it everywhere - #473
Merged
Conversation
The repository carried four different answers to "which Go do we use", and two of them were live at the same time in the same pipeline: CI linted with 1.26.0 via setup-go while Bazel compiled with 1.25.0 via go_sdk.download. The bazel-ci container image ships 1.25.6, and image-credential-helper's go.mod asks for 1.25.11. Nothing reconciled them because each was maintained separately. tools/go-toolchain/go.mod is now the single declaration. It has no source and is never built; it exists so consumers derive the version instead of repeating it: - The root MODULE.bazel reads it via go_sdk.from_file. rules_go requires the file to be named exactly go.mod, which is why the anchor is a module rather than a .go-version file. - Every GitHub Actions setup-go reads it via go-version-file. Five literal pins are gone, three in license-dependencies.yml and two in build-test.yml. Two places cannot derive it and are asserted instead by tools/ci/check-go-version: go.work.bazel, where Bazel requires a literal, and the bazel-ci image, which is built in another repository and so is reported rather than checked. Standardized on 1.26.5, the current stable release. Worth noting the workflows were pinned to 1.26.0, which is five patches behind; "latest" had already drifted. Verified on the derived toolchain: bazel build //... completes over 278 targets, and bazel test //... gives 104 passing with the same 4 failures that occur at origin/main, namely two cloud-tasks Testcontainers suites, go/lib gazelle_test, and icms-translate. tools/scripts/test/test-check-go-version covers the drifts that actually happened: go.work.bazel disagreeing, a workflow reintroducing a literal pin, MODULE.bazel reverting to go_sdk.download, and a missing toolchain directive. Each case was verified to fail the check. Relates to #451 Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe repository centralizes the Go toolchain version in Go toolchain consistency
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 `@tools/ci/check-go-version`:
- Line 42: Make drift detection formatting-independent: in
tools/ci/check-go-version at lines 42-42, inspect every active go-version: key
across both .yml and .yaml workflow files, accepting quoted, double-quoted, and
unquoted values with flexible whitespace; at lines 51-51, detect go_sdk.download
calls with a version argument across arbitrary spacing and line breaks; in
tools/scripts/test/test-check-go-version at lines 48-55, add regression fixtures
covering quoted, unquoted, .yaml, and whitespace/multiline variants.
🪄 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: 33b2680d-4507-4834-a95c-4ba1d1478631
⛔ Files ignored due to path filters (1)
MODULE.bazel.lockis excluded by!**/*.lock,!**/MODULE.bazel.lock
📒 Files selected for processing (8)
.github/workflows/build-test.yml.github/workflows/license-dependencies.ymlMODULE.bazelgo.work.bazeltools/ci/check-go-versiontools/go-toolchain/BUILD.bazeltools/go-toolchain/go.modtools/scripts/test/test-check-go-version
The first version of this change flattened two toolchains into one and would have caused the drift it was meant to prevent. byoo-otel-collector's otelcol genrule shells out to `go build` against $PATH, deliberately, because the collector's 250-module graph does not resolve under rules_go. It therefore compiles with whatever Go the bazel-ci container image ships, and byoo/go.work declares 1.25.6, matching that image. The pairing is not incidental; it is what determines how the shipped collector binary is built. The check previously asserted that the image "must ship Go 1.26.5", which would have broken that pairing and silently recompiled the collector with a different toolchain. It now describes two separate concerns: - The hermetic Bazel SDK, single-sourced in tools/go-toolchain/go.mod at 1.26.5, governing everything built through rules_go. - The host toolchain in the CI image, paired with byoo/go.work at 1.25.6, governing exactly one non-hermetic genrule. The check reports the host requirement rather than asserting equality with the SDK, and the two are explicitly allowed to differ. The requirement is only enforced when byoo-otel-collector is present, so removing that service does not fail the check, while having it without a go.work does. The anchor's own comment made the same conflation and is corrected. Two tests added: a host toolchain differing from the Bazel SDK must be reported and not rejected, and byoo present without a go.work must fail. I found this by checking whether anything consumed the image's Go, concluded from grepping workflow run: steps that nothing did, and was wrong: the consumer is inside a Bazel action reaching out to $PATH, not a workflow step. Relates to #451 Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.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 `@tools/scripts/test/test-check-go-version`:
- Around line 81-84: Strengthen the failure branch in test-check-go-version by
asserting that out contains the specific diagnostic for a missing go.work file
before calling pass. Keep the existing failure expectation, but ensure unrelated
checker errors cannot make the test pass.
🪄 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: 62fee717-a45c-476f-a5c1-dbc831443a93
📒 Files selected for processing (3)
tools/ci/check-go-versiontools/go-toolchain/go.modtools/scripts/test/test-check-go-version
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/go-toolchain/go.mod
- tools/ci/check-go-version
The byoo-without-go.work case passed on any non-zero exit, so an unrelated regression could have kept it green while the missing-file handling was broken. It now asserts the specific diagnostic. Verified by replacing that diagnostic with an unrelated one: the test fails with "checker failed, but not with the missing-go.work diagnostic". Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
kristinapathak
approved these changes
Jul 27, 2026
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.
TL;DR
The repository carried four different answers to "which Go do we use", and two were live in the same pipeline: CI linted with 1.26.0 while Bazel compiled with 1.25.0.
license-dependencies.yml,build-test.ymlsetup-goMODULE.bazelgo_sdk.downloadgo.work.bazelimage-credential-helper/go.modtools/go-toolchain/go.modis now the single declaration, at 1.26.5.Additional Details
The anchor has no source and is never built. It exists so consumers derive the version instead of repeating it:
MODULE.bazelreads it viago_sdk.from_file. rules_go requires the file be named exactlygo.mod(_check_go_mod_name), which is why the anchor is a module rather than a.go-versionfile.setup-goreads it viago-version-file. Five literal pins are gone.Two places genuinely cannot derive it, so
tools/ci/check-go-versionasserts them instead:go.work.bazel, where Bazel requires a literal, and the bazel-ci image, which is built in another repository and is therefore reported rather than checked.Worth noting: the workflows were pinned to 1.26.0, which is five patches behind.
go.devcurrently reports 1.26.5 and 1.25.12. "Latest" had already drifted, which is the failure mode this change is meant to prevent.For the Reviewer
The risk here is the toolchain bump, not the plumbing. Verified on the derived 1.26.5:
bazel build //...completes, 278 targets.bazel test //...gives 104 passing and 4 failing. Those four fail identically atorigin/mainand are unrelated: two cloud-tasks Testcontainers suites,go/lib:gazelle_test, andicms-translate.There are deliberately two toolchains here, and an earlier revision of this PR conflated them. Correcting that is
0bdcb89c.byoo-otel-collector's otelcol genrule shells out togo buildagainst$PATH, on purpose, because the collector's 250-module graph does not resolve under rules_go. It compiles with whatever Go the bazel-ci image ships, andbyoo/go.workdeclares 1.25.6 to match. That pairing determines how the shipped collector binary is built.So:
byoo/go.workat 1.25.6, governs one non-hermetic genrule.The check reports the host requirement rather than asserting equality with the SDK. My first version asserted the image "must ship 1.26.5", which would have broken the pairing and silently recompiled the collector with a different toolchain. The bazel-ci image does not need bumping for this PR.
Service
go.modfiles are also left alone. Theirgodirectives are minimum-version declarations, not toolchain selection, and Go permits them to be lower. Only the toolchain is single-sourced here.For QA
The check runs in the
GitHub release helperjob. Its tests cover the drifts that actually happened:go.work.bazeldisagreeing, a workflow reintroducing a literal,MODULE.bazelreverting togo_sdk.download, and a missing toolchain directive. Each was verified to fail the check.Issues
Relates to #451
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests