Backport PR #2729 to release/v1.7 for Fix installation command for arm64 #2730
Conversation
* 🐛 Fix installation command for arm64 Signed-off-by: vankichi <[email protected]> * 🐛 Fix typo Signed-off-by: vankichi <[email protected]> * 🐛 Fix Signed-off-by: vankichi <[email protected]> --------- Signed-off-by: vankichi <[email protected]>
Deploying vald with
|
| Latest commit: |
659e1ef
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://032f683e.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-bugfix-ja1q.vald.pages.dev |
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to several Makefiles to improve the installation processes for various tools, including Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[CHATOPS:HELP] ChatOps commands.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
Makefile.d/tools.mk (1)
74-75: LGTM! Architecture normalization looks good.The changes correctly handle architecture name normalization for kubectl installation:
- Converts
aarch64toarm64usingDARCHvariable- Ensures consistent architecture naming in download URLs (x86_64 → amd64)
- Maintains lowercase naming convention
Consider extracting the architecture normalization logic into a reusable variable at the top of the Makefile since similar patterns exist in other targets (e.g., go/install). This would improve maintainability:
# Add at the top of the file NORMALIZED_ARCH := $(shell echo $(subst x86_64,amd64,$(subst aarch64,arm64,$(ARCH))) | tr '[:upper:]' '[:lower:]')Then use
$(NORMALIZED_ARCH)consistently across all download URLs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Makefile.d/helm.mk(2 hunks)Makefile.d/k8s.mk(1 hunks)Makefile.d/kind.mk(1 hunks)Makefile.d/tools.mk(1 hunks)
🔇 Additional comments (5)
Makefile.d/kind.mk (1)
22-23: LGTM: Architecture handling is correctly implemented.
The introduction of DARCH variable and the architecture substitution logic properly handles the conversion from aarch64 to arm64, fixing the installation command for ARM architectures while maintaining compatibility with other architectures.
Makefile.d/helm.mk (2)
31-32: LGTM: Correct architecture handling for helm-docs installation
The introduction of DARCH variable properly handles the architecture naming conversion from aarch64 to arm64, ensuring compatibility with helm-docs releases.
31-32: Verify architecture handling across all Makefiles
Let's verify if similar architecture conversion changes are needed in other Makefiles for consistency.
Also applies to: 160-160, 162-162
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the architecture handling across the Makefiles:
Architecture handling is consistently implemented across Makefiles
The codebase already follows a consistent pattern for architecture handling:
- All relevant Makefiles (
helm.mk,tools.mk,minikube.mk,kind.mk,k8s.mk) use the same architecture conversion approach - The pattern
$(subst aarch64,arm64,$(ARCH))is consistently used where needed - Architecture-specific downloads properly handle both
x86_64->amd64andaarch64->arm64conversions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Makefiles that might need similar architecture handling updates
# Look for architecture-related patterns in Makefiles
echo "Searching for architecture-related patterns in Makefiles..."
rg -t make "aarch64|arm64|x86_64|amd64" .
# Look for download URLs in Makefiles that might need architecture handling
echo "Searching for download URLs that might need architecture handling..."
rg -t make "curl.*download.*\$(ARCH)" .
Length of output: 2087
Makefile.d/tools.mk (1)
74-75: Verify architecture handling consistency across Makefiles.
Let's ensure similar architecture normalization is applied consistently across all related Makefiles.
✅ Verification successful
Architecture normalization is consistently implemented across Makefiles
The verification shows consistent architecture handling patterns across the relevant Makefiles:
tools.mk,kind.mk,helm.mk, andk8s.mkall implement the sameDARCHvariable with identicalsubst aarch64,arm64,$(ARCH)normalization- The changes align with the PR objectives of fixing installation commands for arm64 architecture
- The implementation is uniform across the build system, ensuring consistent architecture handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check architecture handling in other Makefiles
# Expected: Similar DARCH variable usage or architecture normalization
echo "Checking architecture handling patterns in Makefiles:"
rg -l "aarch64|arm64|x86_64|amd64" "Makefile.d/"
echo -e "\nChecking specific architecture conversion patterns:"
rg "subst.*aarch64.*arm64" "Makefile.d/"
Length of output: 923
Makefile.d/k8s.mk (1)
537-537: LGTM! Architecture string normalization looks good.
The changes correctly handle architecture string normalization by:
- Converting
aarch64toarm64 - Converting
x86_64toamd64 - Ensuring consistent lowercase architecture strings
This fix ensures the telepresence installation works correctly across different architectures.
Also applies to: 539-539
| $(eval DARCH := $(subst aarch64,arm64,$(ARCH))) | ||
| curl -fsSL https://github.com/kubernetes-sigs/kind/releases/download/v$(KIND_VERSION)/kind-$(OS)-$(subst x86_64,amd64,$(shell echo $(DARCH) | tr '[:upper:]' '[:lower:]')) -o $(BINDIR)/kind |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving readability of the download command.
The current implementation works correctly but could be more maintainable. Consider breaking down the architecture transformations into separate variables.
Here's a suggested refactor:
- $(eval DARCH := $(subst aarch64,arm64,$(ARCH)))
- curl -fsSL https://github.com/kubernetes-sigs/kind/releases/download/v$(KIND_VERSION)/kind-$(OS)-$(subst x86_64,amd64,$(shell echo $(DARCH) | tr '[:upper:]' '[:lower:]')) -o $(BINDIR)/kind
+ $(eval DARCH := $(subst aarch64,arm64,$(ARCH)))
+ $(eval NORMALIZED_ARCH := $(subst x86_64,amd64,$(shell echo $(DARCH) | tr '[:upper:]' '[:lower:]')))
+ curl -fsSL https://github.com/kubernetes-sigs/kind/releases/download/v$(KIND_VERSION)/kind-$(OS)-$(NORMALIZED_ARCH) -o $(BINDIR)/kindThis change:
- Separates architecture normalization into a dedicated variable
- Makes the curl command more readable
- Maintains the same functionality while being easier to maintain
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $(eval DARCH := $(subst aarch64,arm64,$(ARCH))) | |
| curl -fsSL https://github.com/kubernetes-sigs/kind/releases/download/v$(KIND_VERSION)/kind-$(OS)-$(subst x86_64,amd64,$(shell echo $(DARCH) | tr '[:upper:]' '[:lower:]')) -o $(BINDIR)/kind | |
| $(eval DARCH := $(subst aarch64,arm64,$(ARCH))) | |
| $(eval NORMALIZED_ARCH := $(subst x86_64,amd64,$(shell echo $(DARCH) | tr '[:upper:]' '[:lower:]'))) | |
| curl -fsSL https://github.com/kubernetes-sigs/kind/releases/download/v$(KIND_VERSION)/kind-$(OS)-$(NORMALIZED_ARCH) -o $(BINDIR)/kind |
| @@ -156,8 +157,9 @@ yq/install: $(BINDIR)/yq | |||
|
|
|||
| $(BINDIR)/yq: | |||
| mkdir -p $(BINDIR) | |||
| $(eval DARCH := $(subst aarch64,arm64,$(ARCH))) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider standardizing architecture name conversions
While the current implementation works correctly, the architecture name conversions are handled differently between helm-docs and yq installations. Consider standardizing this approach for better maintainability.
You could extract the architecture conversions into a reusable variable at the top of the Makefile:
+ # Architecture name mappings
+ NORMALIZED_ARCH := $(subst x86_64,amd64,$(subst aarch64,arm64,$(ARCH)))
- $(eval DARCH := $(subst aarch64,arm64,$(ARCH)))
- && curl -fsSL https://github.com/mikefarah/yq/releases/download/$(YQ_VERSION)/yq_$(OS)_$(subst x86_64,amd64,$(shell echo $(DARCH) | tr '[:upper:]' '[:lower:]')) -o $(BINDIR)/yq \
+ && curl -fsSL https://github.com/mikefarah/yq/releases/download/$(YQ_VERSION)/yq_$(OS)_$(shell echo $(NORMALIZED_ARCH) | tr '[:upper:]' '[:lower:]') -o $(BINDIR)/yq \This would:
- Centralize architecture name mappings
- Reduce redundancy
- Make future architecture additions easier to manage
Also applies to: 162-162
kpango
deleted the
backport/release/v1.7/bugfix/Makefile/fix-installation-for-arm64
branch
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
New Features
helm-docs,yq,telepresence,kubectl, andkind, ensuring compatibility across different systems.DARCHto dynamically adjust architecture strings based on the system architecture.Bug Fixes