Conversation
|
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 |
|
[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
Codecov Report✅ All modified and coverable lines are covered by tests. Please upload reports for the commit eccd2e8 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## oadp-1.4 #2379 +/- ##
=========================================
Coverage 39.37% 39.37%
=========================================
Files 30 30
Lines 5211 5211
=========================================
Hits 2052 2052
Misses 2959 2959
Partials 200 200 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ifeq evaluates its $(shell ...) condition at parse time, before any target's prerequisites run. On a cold bin/ (setup-envtest not yet installed), `$(ENVTEST) list` fails silently, the grep finds nothing, and ENVTESTPATH gets permanently redefined to force --arch=amd64 -- regardless of host arch -- even though setup-envtest is correctly installed for the host's native arch by the time the `test` recipe actually runs. Move the fallback entirely into the shell expression itself (still a recursively-expanded variable, so it's only evaluated when referenced in the `test` recipe, after `envtest` has installed the right arch). Fixes openshift#2377 Signed-off-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
96a505d to
d406d1d
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address the fallback failure under .SHELLFLAGS = -ec and avoid removing valid cached binaries on every invocation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Fixes envtest architecture resolution by deferring it until recipe execution.
Changes:
- Adds runtime native-architecture resolution with fallback.
- Updates envtest installation and validation prerequisites.
| File | Summary |
|---|---|
Makefile |
Updates envtest path resolution and installation checks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…p-1.5/1.6/dev Copilot flagged two real bugs in the previous ENVTESTPATH/check-envtest-arch approach: - The native-use command substitution's own nonzero exit (under this Makefile's .SHELLFLAGS = -ec) aborted the recipe before the amd64 fallback check ran. - check-envtest-arch treated setup-envtest's documented --help exit code 2 as a broken binary, deleting and reinstalling a perfectly good cached binary on every invocation. Rather than patching around both, adopt the simpler mechanism oadp-1.5/oadp-1.6/oadp-dev already use: resolve KUBEBUILDER_ASSETS inline in the test recipe via '$(ENVTEST) use ... --bin-dir $(LOCALBIN) -p path', after $(ENVTEST) is guaranteed installed for the host's real arch. This drops the separate ENVTESTPATH variable and check-envtest-arch target entirely, fixing openshift#2377 without introducing either of Copilot's flagged issues. Verified locally on arm64: ENVTESTPATH resolves to bin/k8s/<version>-darwin-arm64 (native), and go build ./... passes clean. > [!Note] > Responses generated with Hermes Agent
|
/retest |
|
@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. |
Fix ENVTESTPATH arch decided at Makefile-parse time on cold bin/
Fixes #2377
ENVTESTPATHwas previously computed via a makeifeq, which evaluatesits
$(shell ...)condition at Makefile-parse time, before any target'sprerequisites run. On a cold
bin/(setup-envtestnot yet installed),$(ENVTEST) listfailed silently, the grep found nothing, andENVTESTPATHgot permanently redefined to force--arch=amd64—regardless of host arch — even though
setup-envtestis correctlyinstalled for the host's native arch by the time the
testrecipeactually runs.
KUBEBUILDER_ASSETSis now resolved inline in thetestrecipe itself,via
$(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path— the same mechanismoadp-1.5/oadp-1.6/oadp-devalready use.This only runs after
envtest(a prerequisite oftest) has installedthe right arch, and it lets
setup-envtestresolve its own native-archassets directly against
$(LOCALBIN)rather than this Makefilemaintaining a separate arch-fallback variable, removing the
parse-time/run-time ordering problem entirely.
Kept separate from #2368 (already approved) since that PR's body
explicitly scoped this exact bug out on purpose ("tracked as #2377, not
fixed here... out of scope rather than a reason to expand this PR's diff
a third time") — not reopening that scope decision here.
oadp-1.3 has the same bug but the fix doesn't cherry-pick cleanly
(predates the
go-install-tool-versionedrefactor from #2368), so it'sfixed separately in #2378 rather than via cherry-pick bot.
Testing
Verified locally on arm64:
KUBEBUILDER_ASSETSresolves tobin/k8s/<version>-darwin-arm64(native, not the previous incorrectamd64 fallback), and
go build ./...passes clean.Note
Responses generated with Claude and Hermes Agent