Skip to content

perf(ci): replace -X ldflags with go:embed for version data#19643

Closed
davdhacs wants to merge 1 commit intodavdhacs/fix-test-ldflagsfrom
davdhacs/embed-versions
Closed

perf(ci): replace -X ldflags with go:embed for version data#19643
davdhacs wants to merge 1 commit intodavdhacs/fix-test-ldflagsfrom
davdhacs/embed-versions

Conversation

@davdhacs
Copy link
Copy Markdown
Contributor

@davdhacs davdhacs commented Mar 27, 2026

Description

Go's link ActionID includes -X ldflags verbatim. Today go-tool.sh passes per-commit values (full git describe output 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 -X flags. Per-commit values (full version string, git short SHA) are written to untracked files and loaded at runtime via lazy go: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, writes MAIN_VERSION and GIT_SHORT_SHA_VERSION as untracked files.

version_data.go: GetMainVersion() and GetGitShortSha() lazily read from embed.FS on 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_VERSION glob when dynamic files are absent (fresh clone, go vet, tests).

User-facing documentation

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • modified existing tests

How I validated my change

  • CI green on seed run (all 11 unit-test jobs pass)
  • local-roxctl-tests confirms version info is set correctly for builds
  • go build and go vet work without go-tool.sh (embed glob matches EMPTY_SHA_VERSION)

Partially generated by AI.

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Mar 27, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 32 to 38
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

@rhacs-bot
Copy link
Copy Markdown
Contributor

rhacs-bot commented Mar 27, 2026

Images are ready for the commit at 3cf2b56.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.11.x-448-g837084902b.

@davdhacs davdhacs force-pushed the davdhacs/embed-versions branch from dc57304 to 6f19a29 Compare March 27, 2026 13:48
@github-actions github-actions bot added the ci-all-qa-tests Tells CI to run all API tests (not just BAT). label Mar 27, 2026
@davdhacs davdhacs force-pushed the davdhacs/embed-versions branch 2 times, most recently from 1bc7bf8 to 97721fc Compare March 27, 2026 15:20
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 27, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 49.28%. Comparing base (fbe04ef) to head (d56de76).

Files with missing lines Patch % Lines
pkg/version/version.go 50.00% 1 Missing ⚠️
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     
Flag Coverage Δ
go-unit-tests 49.28% <50.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@davdhacs davdhacs force-pushed the davdhacs/embed-versions branch from e501486 to 3df2803 Compare March 27, 2026 18:07
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fda401b1-52a6-4cc1-a87a-a4d0b97bc4a4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch davdhacs/embed-versions

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@davdhacs davdhacs force-pushed the davdhacs/embed-versions branch from 32200db to dc28f67 Compare March 27, 2026 22:49
@davdhacs davdhacs changed the base branch from master to davdhacs/fix-test-ldflags March 27, 2026 22:49
@davdhacs davdhacs force-pushed the davdhacs/embed-versions branch 7 times, most recently from 3cf2b56 to 8c7e907 Compare March 28, 2026 04:12
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>
@davdhacs davdhacs force-pushed the davdhacs/embed-versions branch from 8c7e907 to d56de76 Compare March 28, 2026 04:16
@davdhacs
Copy link
Copy Markdown
Contributor Author

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:

  1. Link ActionID (linkActionID) — includes linkflags %q at line 1404. Changing -X ldflags changes this ID.

  2. Compile ActionID — does NOT include ldflags. Compilation is fully cached regardless of ldflag changes.

  3. Linked binary cache — Go explicitly does not cache linked binaries in GOCACHE for go build (only for go run). The on-disk target check (line 465) doesn't help in CI with fresh checkouts.

  4. Test cache — test result caching (Phase 1) depends on the test binary identity, which includes the link ActionID. This is where changing ldflags actually hurts. Fixed by perf(ci): use fixed version ldflags for tests to enable GOCACHE hits #19617 with stable test-only ldflags.

The status.sh overhead (~15s) and -buildvcs=false improvements are already addressed by #19585 (merged).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants