Remove setup-envtest if different arch during make test - #2152
Conversation
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughModified the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ 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
There was a problem hiding this comment.
Pull request overview
This PR aims to make make test more reliable by automatically cleaning up an incompatible setup-envtest binary that may have been written into the shared bin/ directory by a container build targeting a different architecture.
Changes:
- Adds a pre-check in the
setup-envtestinstallation rule to remove an existingsetup-envtestbinary if it can’t be executed on the current platform. - Documents the cross-architecture scenario that can cause
make testfailures due to a non-native binary inbin/.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .PHONY: envtest | ||
| envtest: $(ENVTEST) ## Download envtest-setup locally if necessary. | ||
| $(ENVTEST): $(LOCALBIN) | ||
| @if [ -f $(ENVTEST) ] && ! $(ENVTEST) --help >/dev/null 2>&1; then \ | ||
| echo "Removing incompatible setup-envtest binary"; \ | ||
| rm -f $(ENVTEST); \ | ||
| fi | ||
| $(call go-install-tool,$(ENVTEST),sigs.k8s.io/controller-runtime/tools/setup-envtest@v0.0.0-20250308055145-5fe7bb3edc86) |
There was a problem hiding this comment.
The incompatibility check is inside the $(ENVTEST) file target recipe, but that recipe won’t run when $(ENVTEST) already exists and is considered up-to-date. In the exact scenario this PR describes (a wrong-arch bin/setup-envtest already present), make test will usually skip this recipe and the bad binary won’t be removed. Consider making the compatibility check run unconditionally (e.g., move the check into the .PHONY: envtest target, or add a phony FORCE prerequisite to $(ENVTEST) so the recipe always executes; go-install-tool already avoids re-installing when the file is present).
|
/retest |
|
/override ci/prow/4.23-e2e-test-aws |
|
@kaovilai: Overrode contexts on behalf of kaovilai: ci/prow/4.23-e2e-test-aws DetailsIn response to this:
Instructions 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. |
|
@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. |
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>
|
Folding this into #2367 to reduce review load — closing in favor of that PR, which covers this fix plus the same underlying issue for controller-gen/kustomize/golangci-lint. Along the way I found the Note Responses generated with Claude |
Signed-off-by: Tiger Kaovilai tkaovila@redhat.com
Why the changes were made
How to test the changes made