perf(ci): replace -X ldflags with go:embed for version data#19643
perf(ci): replace -X ldflags with go:embed for version data#19643davdhacs wants to merge 1 commit intodavdhacs/fix-test-ldflagsfrom
Conversation
|
Skipping CI for Draft Pull Request. |
86d52d3 to
8d63a07
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
go:embedapproach inpkg/version/internal/version_data.godepends on the generated files existing; consider ensuring placeholder defaults are checked in or adding a guard/generator hook so commands likego list ./...orgo vetin a clean tree don’t fail whenscripts/go-tool.shhasn’t been run. - In
scripts/go-tool.sh, thegit describe/git rev-parsefallbacks assume a git repo and tags are always available; it may be worth adding explicit error handling or a clearer fallback for detached/untagged or non-git build environments. - The change in
.github/actions/cache-go-dependencies/action.yamlto always save the Go build cache is marked as temporary; consider gating this behind an input or reverting to conditional save before merging to avoid unnecessary cache uploads on all PRs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `go:embed` approach in `pkg/version/internal/version_data.go` depends on the generated files existing; consider ensuring placeholder defaults are checked in or adding a guard/generator hook so commands like `go list ./...` or `go vet` in a clean tree don’t fail when `scripts/go-tool.sh` hasn’t been run.
- In `scripts/go-tool.sh`, the `git describe`/`git rev-parse` fallbacks assume a git repo and tags are always available; it may be worth adding explicit error handling or a clearer fallback for detached/untagged or non-git build environments.
- The change in `.github/actions/cache-go-dependencies/action.yaml` to always save the Go build cache is marked as temporary; consider gating this behind an input or reverting to conditional save before merging to avoid unnecessary cache uploads on all PRs.
## Individual Comments
### Comment 1
<location path="scripts/go-tool.sh" line_range="32-38" />
<code_context>
+# Determine the version string.
+# BUILD_TAG is set by CI (GHA via define-job-matrix, Konflux via Docker ARG).
+# Falls back to git describe for local development.
+if [[ -n "${BUILD_TAG:-}" ]]; then
+ main_version="${BUILD_TAG}"
+ git_short_sha="$(echo "$BUILD_TAG" | sed -n 's/.*g\([0-9a-f]\{1,\}\)$/\1/p')"
+else
+ main_version="$(cd "${REPO_ROOT}"; git describe --tags --abbrev=10 --long --exclude '*-nightly-*')"
</code_context>
<issue_to_address>
**suggestion:** Consider a fallback for git_short_sha when BUILD_TAG does not contain a git-style suffix
This assumes BUILD_TAG ends with `g<sha>`. If it doesn’t (e.g., different CI format or local override), `git_short_sha` will be empty even in non-test builds, which is a behavior change and reduces usefulness of GitShortSha. Consider detecting an empty result and falling back to `git rev-parse --short HEAD` to keep it populated when possible.
```suggestion
if [[ -n "${BUILD_TAG:-}" ]]; then
main_version="${BUILD_TAG}"
git_short_sha="$(echo "$BUILD_TAG" | sed -n 's/.*g\([0-9a-f]\{1,\}\)$/\1/p')"
if [[ -z "${git_short_sha}" ]]; then
git_short_sha="$(cd "${REPO_ROOT}"; git rev-parse --short HEAD)"
fi
else
main_version="$(cd "${REPO_ROOT}"; git describe --tags --abbrev=10 --long --exclude '*-nightly-*')"
git_short_sha="$(cd "${REPO_ROOT}"; git rev-parse --short HEAD)"
fi
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
scripts/go-tool.sh
Outdated
| if [[ -n "${BUILD_TAG:-}" ]]; then | ||
| main_version="${BUILD_TAG}" | ||
| git_short_sha="$(echo "$BUILD_TAG" | sed -n 's/.*g\([0-9a-f]\{1,\}\)$/\1/p')" | ||
| else | ||
| main_version="$(cd "${REPO_ROOT}"; git describe --tags --abbrev=10 --long --exclude '*-nightly-*')" | ||
| git_short_sha="$(cd "${REPO_ROOT}"; git rev-parse --short HEAD)" | ||
| fi |
There was a problem hiding this comment.
suggestion: Consider a fallback for git_short_sha when BUILD_TAG does not contain a git-style suffix
This assumes BUILD_TAG ends with g<sha>. If it doesn’t (e.g., different CI format or local override), git_short_sha will be empty even in non-test builds, which is a behavior change and reduces usefulness of GitShortSha. Consider detecting an empty result and falling back to git rev-parse --short HEAD to keep it populated when possible.
| if [[ -n "${BUILD_TAG:-}" ]]; then | |
| main_version="${BUILD_TAG}" | |
| git_short_sha="$(echo "$BUILD_TAG" | sed -n 's/.*g\([0-9a-f]\{1,\}\)$/\1/p')" | |
| else | |
| main_version="$(cd "${REPO_ROOT}"; git describe --tags --abbrev=10 --long --exclude '*-nightly-*')" | |
| git_short_sha="$(cd "${REPO_ROOT}"; git rev-parse --short HEAD)" | |
| fi | |
| if [[ -n "${BUILD_TAG:-}" ]]; then | |
| main_version="${BUILD_TAG}" | |
| git_short_sha="$(echo "$BUILD_TAG" | sed -n 's/.*g\([0-9a-f]\{1,\}\)$/\1/p')" | |
| if [[ -z "${git_short_sha}" ]]; then | |
| git_short_sha="$(cd "${REPO_ROOT}"; git rev-parse --short HEAD)" | |
| fi | |
| else | |
| main_version="$(cd "${REPO_ROOT}"; git describe --tags --abbrev=10 --long --exclude '*-nightly-*')" | |
| git_short_sha="$(cd "${REPO_ROOT}"; git rev-parse --short HEAD)" | |
| fi |
|
Images are ready for the commit at 3cf2b56. To use with deploy scripts, first |
dc57304 to
6f19a29
Compare
1bc7bf8 to
97721fc
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## davdhacs/fix-test-ldflags #19643 +/- ##
=============================================================
- Coverage 49.29% 49.28% -0.01%
=============================================================
Files 2735 2735
Lines 206213 206215 +2
=============================================================
- Hits 101643 101633 -10
- Misses 97029 97040 +11
- Partials 7541 7542 +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:
|
e501486 to
3df2803
Compare
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
32200db to
dc28f67
Compare
3cf2b56 to
8c7e907
Compare
Remove //XDef: from MainVersion and GitShortSha so they are no longer in ldflags (which change per commit, breaking link cache). Instead, go-tool.sh writes per-commit values to untracked files loaded at runtime via lazy go:embed. The remaining ldflags (CollectorVersion, ScannerVersion, FactVersion) are stable across commits. Also adds -buildvcs=false to prevent Go VCS stamps from changing link ActionIDs per commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8c7e907 to
d56de76
Compare
|
Closing — stable build ldflags don't improve build caching because Go doesn't cache linked binaries in GOCACHE. What ldflags actually affect in Go's cache:
The |
Description
Go's link ActionID includes
-Xldflags verbatim. Todaygo-tool.shpasses per-commit values (fullgit describeoutput with commit count and SHA), so every commit invalidates the link cache for all binaries.This PR stabilizes build ldflags by using only the base tag (
git describe --abbrev=0) in-Xflags. Per-commit values (full version string, git short SHA) are written to untracked files and loaded at runtime via lazygo:embed+sync.Once.Depends on #19617 for the test-side ldflags fix.
How it works
go-tool.sh: ldflags use base tag for
MainVersion(stable across commits). For builds only, writesMAIN_VERSIONandGIT_SHORT_SHA_VERSIONas untracked files.version_data.go:
GetMainVersion()andGetGitShortSha()lazily read fromembed.FSon first call. For tests (where files don't exist), falls back to ldflag values.EMPTY_SHA_VERSION: committed empty file satisfies the
//go:embed *_SHA_VERSIONglob when dynamic files are absent (fresh clone,go vet, tests).User-facing documentation
Testing and quality
Automated testing
How I validated my change
local-roxctl-testsconfirms version info is set correctly for buildsgo buildandgo vetwork without go-tool.sh (embed glob matches EMPTY_SHA_VERSION)Partially generated by AI.