perf(ci): use stable version ldflags for test caching#19617
Conversation
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at dba861f. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19617 +/- ##
=======================================
Coverage 49.66% 49.66%
=======================================
Files 2748 2748
Lines 207354 207354
=======================================
+ Hits 102987 102991 +4
+ Misses 96711 96708 -3
+ Partials 7656 7655 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
davdhacs
force-pushed
the
davdhacs/fix-test-ldflags
branch
from
fdfe8cb to
fbe04ef
Compare
Tests don't need real version info. Use fixed version strings (0.0.0-test) instead of git-describe values for test ldflags. This makes link ActionIDs stable across commits, enabling Go's test result cache to hit. Also adds -buildvcs=false to stop Go 1.18+'s VCS stamping which independently changes link ActionIDs on every commit. Builds keep the current XDef/status.sh mechanism unchanged. Verified locally: tests cache across commits with fixed ldflags. Combined with the mtime fix (#19395), this enables full warm test caching in CI (2.5-8.2x faster test jobs). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
davdhacs
force-pushed
the
davdhacs/fix-test-ldflags
branch
from
fbe04ef to
1e6afe7
Compare
|
This change is kind of complex. How about setting |
@janisz You're absolutely right. Thank you. I hadn't seen that instead of the if/else in go-tool.sh, I could just set BUILD_TAG and SHORTCOMMIT. And that removes it from happening for local-dev. For the go:embed, that would require copying the *_VERSION files into the version subdir. So that gets complex and also has a downside of affecting the .go file content which would change cache keys (rarely changing but invalidating the compilation cache for everything that directly or indirectly imports it) (my experiment with go:embed: #19643). I'll update this PT to the better simply-set vars approach you've suggested. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The repeated
BUILD_TAG/SHORTCOMMITenv configuration across multiple jobs could be centralized at the workflow level (or via a reusable workflow) to reduce duplication and the risk of the values drifting in the future. - Only the first job has an explanatory comment about why
BUILD_TAG/SHORTCOMMITare fixed; consider adding a short similar comment near the other jobs using these env vars to make the intent clear when reading those sections in isolation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated `BUILD_TAG`/`SHORTCOMMIT` env configuration across multiple jobs could be centralized at the workflow level (or via a reusable workflow) to reduce duplication and the risk of the values drifting in the future.
- Only the first job has an explanatory comment about why `BUILD_TAG`/`SHORTCOMMIT` are fixed; consider adding a short similar comment near the other jobs using these env vars to make the intent clear when reading those sections in isolation.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
davdhacs
changed the title
📝 WalkthroughWalkthroughAdded job-level environment variables Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/unit-tests.yaml (2)
406-409: Same suggestion: add a brief comment for consistency.The
sensor-integration-testsjob already had anenvblock forKUBECONFIG, and the new variables are correctly appended. Similar togo-postgres, a brief comment would improve discoverability.💡 Suggested comment
sensor-integration-tests: env: KUBECONFIG: "/tmp/kubeconfig" + # See 'go' job comment: fixed version ldflags for test caching. BUILD_TAG: 0.0.0 SHORTCOMMIT: "0000000"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yaml around lines 406 - 409, Add a short inline comment above the sensor-integration-tests job's env block explaining why KUBECONFIG, BUILD_TAG and SHORTCOMMIT are set (same style as the comment used in go-postgres) to improve discoverability; locate the env block that defines KUBECONFIG: "/tmp/kubeconfig", BUILD_TAG: 0.0.0 and SHORTCOMMIT: "0000000" and insert a concise comment describing their purpose (e.g., kube config path and build metadata) on the line(s) immediately preceding that env block.
112-114: Consider adding a brief comment for maintainability.The
go-postgresjob sets the same env vars as thegojob but lacks the explanatory comment. A short reference would help future maintainers understand why these values are fixed here.💡 Suggested comment
+ # See 'go' job comment: fixed version ldflags for test caching. env: BUILD_TAG: 0.0.0 SHORTCOMMIT: "0000000"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yaml around lines 112 - 114, Add a short explanatory comment above the env block (or next to BUILD_TAG/SHORTCOMMIT) clarifying that BUILD_TAG and SHORTCOMMIT are intentionally fixed to "0.0.0" and "0000000" for reproducible unit-test runs and to match the values used in the go job; reference the env variables BUILD_TAG and SHORTCOMMIT so future maintainers understand this mirroring and why the values are hard-coded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/unit-tests.yaml:
- Around line 406-409: Add a short inline comment above the
sensor-integration-tests job's env block explaining why KUBECONFIG, BUILD_TAG
and SHORTCOMMIT are set (same style as the comment used in go-postgres) to
improve discoverability; locate the env block that defines KUBECONFIG:
"/tmp/kubeconfig", BUILD_TAG: 0.0.0 and SHORTCOMMIT: "0000000" and insert a
concise comment describing their purpose (e.g., kube config path and build
metadata) on the line(s) immediately preceding that env block.
- Around line 112-114: Add a short explanatory comment above the env block (or
next to BUILD_TAG/SHORTCOMMIT) clarifying that BUILD_TAG and SHORTCOMMIT are
intentionally fixed to "0.0.0" and "0000000" for reproducible unit-test runs and
to match the values used in the go job; reference the env variables BUILD_TAG
and SHORTCOMMIT so future maintainers understand this mirroring and why the
values are hard-coded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: dbd7bdaf-6705-4b88-837c-7dc0149f7d93
📒 Files selected for processing (1)
.github/workflows/unit-tests.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/cache-go-dependencies/action.yaml:
- Around line 26-34: The cache save step currently uses if: inputs.save ==
'true' and writes SHA-scoped caches (the key includes github.sha), causing
churn; update the actions/cache@v5 save branch to only run when inputs.save is
true AND the workflow is a push to the repository default branch (e.g.,
github.event_name == 'push' and github.ref matches the default branch), so only
default-branch pushes create cache entries for the key (go-build-v1-${{
github.job }}...${{ steps.cache-paths.outputs.TAG }}); keep the existing
restore-only step for other events/branches and ensure you do not remove the
restore logic.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 40bf3b48-86eb-4bd4-9ef9-24ec18e634c9
📒 Files selected for processing (1)
.github/actions/cache-go-dependencies/action.yaml
davdhacs
force-pushed
the
davdhacs/fix-test-ldflags
branch
from
66f9a38 to
901e0ca
Compare
Set BUILD_TAG=0.0.0 and SHORTCOMMIT=0000000 on Go test jobs so that version ldflags are stable across commits. Go's test cache keys include the link ActionID which includes -X ldflags; per-commit values (git describe, short SHA) cause every test to miss the cache. Applied to: go, go-postgres, sensor-integration-tests. Skipped: local-roxctl-tests (needs real version for assertions), go-bench (benchmarks are uncacheable). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
davdhacs
force-pushed
the
davdhacs/fix-test-ldflags
branch
from
901e0ca to
dba861f
Compare
|
@janisz how does this look? |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider scoping
BUILD_TAGandSHORTCOMMITto the specific steps that rungo testrather than the whole job, to avoid accidentally affecting any future steps in these jobs that might need the real version metadata. - The hard-coded placeholder values (
0.0.0and0000000) are a bit opaque; adding a brief note that these must remain stable and unused by any test assertions will help prevent future changes from unintentionally depending on them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider scoping `BUILD_TAG` and `SHORTCOMMIT` to the specific steps that run `go test` rather than the whole job, to avoid accidentally affecting any future steps in these jobs that might need the real version metadata.
- The hard-coded placeholder values (`0.0.0` and `0000000`) are a bit opaque; adding a brief note that these must remain stable and unused by any test assertions will help prevent future changes from unintentionally depending on them.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
3 participants
Description
Set
BUILD_TAG=0.0.0andSHORTCOMMIT=0000000as env vars on Go test jobs so that version ldflags are stable across commits.Go's test cache keys include the link ActionID, which includes
-Xldflags. Per-commit values fromstatus.sh(git describe, short SHA) cause every test to miss the cache. With stable values, test results cache across commits.Collector/scanner/fact versions still use real values from
status.sh— they change rarely (~weekly) and are acceptable cache-miss sources.Applied to:
go,go-postgres,sensor-integration-tests.Skipped:
local-roxctl-tests(needs real version for assertions),go-bench(benchmarks are uncacheable).Background
go:embed(perf(ci): replace -X ldflags with go:embed for version data #19643) but abandoned — it changes compile-time contentID, causing cascading recompilation of all downstream packages. Ldflags are applied at link time only, keeping compile outputs stable.if/elserewrite ingo-tool.shto env vars per janisz's suggestion.Previous experiment results
With stable ldflags + stable mtimes, Go unit tests went from ~33m to ~7m (warm) on PR branches. See #19618 for full benchmarks.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
CI validation. The env vars affect only the version strings passed via
-Xldflags to test binaries — no test logic depends on real version values.🤖 Generated with Claude Code