Make controller-gen/kustomize/golangci-lint/envtest tool-binary caching reliable - #2367
Open
kaovilai wants to merge 2 commits into
Open
Make controller-gen/kustomize/golangci-lint/envtest tool-binary caching reliable#2367kaovilai wants to merge 2 commits into
kaovilai wants to merge 2 commits into
Conversation
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. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Contributor
WalkthroughThe Makefile now installs pinned development tools with version-marker files. It replaces mismatched binaries, preserves unversioned symlinks, and removes ChangesTool installation and compatibility
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@Makefile`:
- Around line 530-550: Update the versions target to read the .version sidecar
for marker-managed tools such as KUSTOMIZE instead of invoking the binary’s
version command. Preserve existing binary introspection for tools without marker
files, and reuse the marker path established by go-install-tool-versioned.
🪄 Autofix
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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e93a7bca-24c0-4d5d-82da-82c240a84ce2
📒 Files selected for processing (1)
Makefile
Comment on lines
+530
to
+550
| # go-install-tool-versioned installs $2 to branch-specific path $1, but only if $1 is missing | ||
| # or $1.version doesn't match the pinned version $3. Uses a sidecar marker file instead of | ||
| # introspecting the binary's own --version output, because that output is unreliable for some | ||
| # tools when installed via `go install` (e.g. kustomize reports "(devel)" or an unexpanded | ||
| # `$$Format:%H$$` placeholder instead of its real version, depending on build-time factors). | ||
| define go-install-tool-versioned | ||
| @if [ -f $(1) ] && [ -f $(1).version ] && [ "$$(cat $(1).version)" = "$(3)" ]; then \ | ||
| echo "$(notdir $(1)) $(3) is already installed" ;\ | ||
| else \ | ||
| set -e ;\ | ||
| mkdir -p $(dir $(1)) ;\ | ||
| rm -f $(1) $(1).version ;\ | ||
| TMP_DIR=$$(mktemp -d) ;\ | ||
| cd $$TMP_DIR ;\ | ||
| go mod init tmp ;\ | ||
| echo "Installing $(notdir $(1)) $(3)" ;\ | ||
| GOBIN=$(dir $(1)) go install -a -mod=mod $(2) ;\ | ||
| cd - >/dev/null ;\ | ||
| rm -rf $$TMP_DIR ;\ | ||
| echo "$(3)" > $(1).version ;\ | ||
| fi |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the sidecar marker in the versions target.
Lines 530-534 state that go install can make Kustomize report (devel). Line 167 still obtains the Kustomize version from $(KUSTOMIZE) version --short. A correct $(KUSTOMIZE).version marker can therefore coexist with a false version report from make versions.
Read the sidecar marker for marker-managed tools. Preserve binary introspection only for tools that do not use a marker.
🤖 Prompt for 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.
In `@Makefile` around lines 530 - 550, Update the versions target to read the
.version sidecar for marker-managed tools such as KUSTOMIZE instead of invoking
the binary’s version command. Preserve existing binary introspection for tools
without marker files, and reuse the marker path established by
go-install-tool-versioned.
Folds in the fix from openshift#2152 (same author, same theme: harden Makefile tool-binary caching under bin/). A containerized Make target (e.g. podman/docker build with a different GOARCH) can write a linux binary into the shared bin/ directory, replacing the native host binary `make test` needs — and since setup-envtest isn't branch-scoped like the other three tools, that binary is shared across every branch checkout too. openshift#2152's own check used `$(ENVTEST) --help`'s exit code as the "is this binary compatible" signal, but that's unreliable the same way relying on kustomize's --version output was: setup-envtest's own --help exits 2 by its own convention even on a perfectly good binary, so that check would have triggered a reinstall on every single invocation, permanently defeating the cache. 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) — check that instead of any nonzero exit. Also added $(ENVTEST) to the .PHONY line, matching the fix already applied to controller-gen/kustomize in the previous commit: without it, Make's own mtime-based staleness check can skip the recipe (and therefore this check) entirely once the binary file exists. Closes openshift#2152 Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
kaovilai
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
544-566: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake marker-managed caches platform-specific.
go-install-tool-versionedchecks only the version marker. The cache paths for Controller-Gen, Kustomize, and GolangCI-Lint omitGOOSandGOARCH. A same-version binary from another architecture can be reused and fail at execution. Include the platform in the cache key or validate the binary before reuse.🤖 Prompt for 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. In `@Makefile` around lines 544 - 566, The go-install-tool-versioned cache currently reuses markers based only on tool version, allowing binaries from another platform to be selected. Update go-install-tool-versioned and the Controller-Gen, Kustomize, and GolangCI-Lint cache paths or marker validation to include GOOS and GOARCH, ensuring reuse only occurs for the current platform while preserving version checks.
🤖 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.
Outside diff comments:
In `@Makefile`:
- Around line 544-566: The go-install-tool-versioned cache currently reuses
markers based only on tool version, allowing binaries from another platform to
be selected. Update go-install-tool-versioned and the Controller-Gen, Kustomize,
and GolangCI-Lint cache paths or marker validation to include GOOS and GOARCH,
ensuring reuse only occurs for the current platform while preserving version
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5cedc214-40c1-438d-a936-5286a91d2bf1
📒 Files selected for processing (1)
Makefile
|
@kaovilai: The following test failed, say
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. |
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
1 participant
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.
Folds in #2152 (same author, same theme, reviewer bandwidth is thin — consolidating into one PR rather than two small ones).
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 across branch switches in the same checkout, even afterCONTROLLER_TOOLS_VERSION/KUSTOMIZE_VERSION/GOLANGCI_LINT_VERSIONchange — becausebin/is gitignored and nothing else resets it. Thecontroller-gen/kustomizetarget help text even claims otherwise ("If wrong version is installed, it will be overwritten/removed"), but the recipes didn't actually do that.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.I hit this while investigating an unexpectedly large
make manifestsreformat diff onoadp-1.4— traced it back to a stale cachedcontroller-gen/kustomizebinary in a long-lived local checkout.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. Binary introspection isn't reliable for every tool 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 correctv5.2.1respectively.Part 2: envtest arch-check (folds in #2152)
Same root cause, different failure mode:
setup-envtestisn't even branch-scoped (ENVTEST ?= $(LOCALBIN)/setup-envtest, no$(BRANCH_VERSION)), so it's shared across every branch checkout too. A containerized Make target (podman/docker build with a differentGOARCH) can write a linux binary intobin/, replacing the native host binarymake testneeds.#2152 proposed removing+reinstalling the binary when
$(ENVTEST) --helpfails — but I found that check unreliable the same way binary---versionintrospection was unreliable for kustomize:setup-envtest's own--helpexits2by its own convention even on a perfectly working binary, so that check would trigger a reinstall on every single invocation, permanently defeating the cache (verified: it printed "Removing incompatible setup-envtest binary" immediately after a fresh, working install).Fixed version checks specifically for exit code
126(POSIX "found but cannot execute" / exec format error), not any nonzero exit — verified by cross-compiling a reallinux/amd64setup-envtestand running it on adarwin/arm64host: the shell reports exit126distinctly from the tool's own exit codes. Also added$(ENVTEST)to.PHONY, matching the fix in Part 1 (same Make-recipe-skip issue).How I tested (both parts)
rm -rf bin/oadp-devthenmake controller-gen/make kustomize/make golangci-lint/make envtestindividually — each installs fresh (writing a.versionmarker for the first three)..versionmarker) for controller-gen/kustomize/golangci-lint → 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 lint→ 0 issues.make test→ exit 0, all packages pass,api is up to date,bundle is up to date.Plan to cherry-pick this to
oadp-1.4(already have #2368 open there for Part 1; will fold Part 2 in as well).Note
Responses generated with Claude
Summary by CodeRabbit