[oadp-1.4] Make controller-gen/kustomize/golangci-lint/envtest tool-binary caching reliable - #2368
[oadp-1.4] Make controller-gen/kustomize/golangci-lint/envtest tool-binary caching reliable#2368kaovilai wants to merge 2 commits into
Conversation
Cherry-pick of the fix from oadp-dev. go-install-tool-branch only installs when the binary is missing, never verifying the pinned version against what's already on disk. Once a binary lands in bin/<branch>/, it's reused forever across branch switches and version bumps since bin/ is gitignored and nothing else resets it. kustomize and controller-gen's targets also weren't fully .PHONY (only the wrapper name was, not the binary path), so Make's own mtime-based staleness check could skip their recipe entirely before any version check ran. Introduce go-install-tool-versioned, which compares a sidecar <binary>.version marker file against the pinned version instead of introspecting the binary's own --version output. Binary introspection isn't reliable for every tool installed this way: kustomize's `version` command depends on ldflags its own release process sets, which `go install` doesn't set, so identically-installed kustomize binaries were observed reporting "(devel)", an unexpanded `$Format:%H$` git-archive placeholder, or a correct version string depending on unrelated build-time factors. oadp-1.4's golangci-lint target didn't have any version-check at all (unlike oadp-dev's, which had a partial/broken one), so it's folded into the same fix here. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
openshift-ci
Bot
added
the
approved
Codecov Report❌ Patch coverage is Please upload reports for the commit af5468b to get more accurate results.
Additional details and impacted files@@ Coverage Diff @@
## oadp-1.4 #2368 +/- ##
============================================
+ Coverage 37.79% 38.47% +0.67%
============================================
Files 30 30
Lines 5033 5113 +80
============================================
+ Hits 1902 1967 +65
- Misses 2935 2949 +14
- Partials 196 197 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch's check-envtest-arch (landed via openshift#2100, Feb 2026) has been unconditionally reinstalling setup-envtest on every single `make test`/`make envtest` invocation since it merged: it treats any `$(ENVTEST) --help` failure as "wrong architecture," but setup-envtest's own --help exits 2 by its own convention even on a perfectly working binary. Since check-envtest-arch is itself .PHONY and a prerequisite of $(ENVTEST), that always-nonzero exit forces a delete+reinstall on every invocation, unconditionally, defeating the point of caching this binary at all. Verified by cross-compiling a real linux/amd64 setup-envtest and running it on this darwin/arm64 host: the shell reports exit code 126 specifically (POSIX "found but cannot execute" / exec format error) for a genuinely incompatible binary, distinct from the tool's own exit codes. Check that instead. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@kaovilai: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Cherry-pick of #2367 to
oadp-1.4— same underlying issue, same fix, plus a live bug I found already merged on this branch.Part 1: version-checking for controller-gen/kustomize/golangci-lint
go-install-tool-branchonly installs a build tool (controller-gen, kustomize, golangci-lint) when the binary is missing, never verifying the pinned version against what's already on disk. Once a binary lands atbin/<branch>/<tool>, it's reused forever — even afterCONTROLLER_TOOLS_VERSION/KUSTOMIZE_VERSION/GOLANGCI_LINT_VERSIONchange — becausebin/is gitignored and nothing else resets it.kustomize/controller-gentargets also weren't fully.PHONY(only the wrapper name was, not the binary path), so Make's own mtime-based staleness check could skip the recipe entirely before any version-check logic even ran. (oadp-1.4'sgolangci-linttarget didn't have any version-check at all, unlikeoadp-dev's partial one — folded into this same fix.)Fix: added
go-install-tool-versioned, which compares a sidecar<binary>.versionmarker file against the pinned version instead of introspecting the binary's own--versionoutput — unreliable for some tools installed this way. Confirmed in practice: threebin/*/kustomizebinaries on this machine, all installed the same way, reported(devel), a literal unexpanded$Format:%H$git-archive placeholder, and a correct version string respectively.Part 2: envtest arch-check (already-live bug fix)
This branch already has an envtest arch-check (
check-envtest-arch, landed via #2100 back in Feb 2026) — but it's been unconditionally reinstallingsetup-envteston every singlemake test/make envtestinvocation since it merged. It treats any$(ENVTEST) --helpfailure as "wrong architecture," butsetup-envtest's own--helpexits2by its own convention even on a perfectly working binary. Sincecheck-envtest-archis itself.PHONYand a prerequisite of$(ENVTEST), that always-nonzero exit forces a delete+reinstall every time, unconditionally — defeating the point of caching this binary at all.Fixed to check exit code
126specifically (POSIX "found but cannot execute" / exec format error), not any nonzero exit — verified by cross-compiling a reallinux/amd64setup-envtestand running it on thisdarwin/arm64host: the shell reports exit126distinctly from the tool's own exit codes.How I tested (both parts)
rm -rf bin/oadp-1.4/rm -f bin/setup-envtestthenmake controller-gen/make kustomize/make envtestindividually — each installs fresh.make envteston this branch would always redownload — confirmed before my fix.).versionmarker) for controller-gen/kustomize → correctly detected, removed, reinstalled, marker rewritten.linux/amd64, copied over the native binary) → correctly detected via exit 126, removed, reinstalled with a valid native binary.make generate manifests bundle→ zero diff against a clean checkout.make test→ exit 0, all packages pass,Go code is formatted,api is up to date,bundle is up to date.Note:
golangci-lint@v1.54.2(this branch's pin) fails to build against a modern Go toolchain (Go 1.26) in my local sandbox due to an oldx/toolstransitive dependency incompatibility unrelated to this change.make teston this branch doesn't invokegolangci-lintat all, so this didn't block verification; CI pins its own Go version and isn't affected.Note
Responses generated with Claude