Fix installation command for arm64 #2729
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces modifications to several Makefile scripts, enhancing the installation processes for various tools by improving architecture handling. A new variable, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 3
🧹 Outside diff range and nitpick comments (1)
Makefile.d/tools.mk (1)
74-78: LGTM! Architecture handling looks correct.The implementation properly handles the conversion from
aarch64toarm64for kubectl installation, which aligns with the PR objective.Consider consolidating the architecture conversion logic since similar conversions exist in other targets (e.g., go/install). You could create a reusable variable at the top of the Makefile:
# At the top of the file or in a common include NORMALIZED_ARCH := $(subst x86_64,amd64,$(subst aarch64,arm64,$(ARCH))) # Then use it in the targets $(BINDIR)/kubectl: curl -fsSL "https://dl.k8s.io/release/$(KUBECTL_VERSION)/bin/$(OS)/$(NORMALIZED_ARCH)/kubectl" -o $(BINDIR)/kubectl
📜 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 (6)
Makefile.d/kind.mk (1)
21-26: Verify arm64 binary availability.
The changes assume the availability of arm64 binaries for KinD. Let's verify this assumption.
✅ Verification successful
Let me verify the actual release assets for KinD v0.20.0 to ensure we're using the correct binary name.
arm64 binary availability verified for KinD
The changes are correct. The KinD v0.20.0 release includes the kind-linux-arm64 binary, confirming that the Makefile's arm64 architecture handling is properly implemented. The binary naming convention in the Makefile matches the actual release assets.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the availability of arm64 binaries for the specified KIND_VERSION
# Test: Check if the arm64 binary exists
KIND_VERSION=$(grep "KIND_VERSION" .env 2>/dev/null || echo "0.20.0")
HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code}" \
"https://github.com/kubernetes-sigs/kind/releases/download/v${KIND_VERSION}/kind-linux-arm64")
if [ "$HTTP_CODE" = "200" ]; then
echo "✅ arm64 binary is available for KinD v${KIND_VERSION}"
else
echo "❌ arm64 binary not found for KinD v${KIND_VERSION}"
fi
Length of output: 316
Script:
#!/bin/bash
# Check the actual release assets for KinD v0.20.0
curl -s https://api.github.com/repos/kubernetes-sigs/kind/releases/tags/v0.20.0 | jq -r '.assets[].name'
Length of output: 334
Makefile.d/helm.mk (2)
30-35: LGTM: Architecture handling for helm-docs looks correct.
The architecture conversion from aarch64 to arm64 is properly implemented and matches helm-docs release naming convention.
Let's verify the helm-docs release artifacts naming:
#!/bin/bash
# Description: Verify helm-docs release artifacts naming convention
# Expected: Should show releases containing "arm64" not "aarch64"
curl -s https://api.github.com/repos/norwoodj/helm-docs/releases/latest | \
grep -i "browser_download_url.*linux.*" | \
cut -d '"' -f 4162-168: 🛠️ Refactor suggestion
Simplify architecture handling in yq installation.
While the current implementation works, it can be simplified to improve maintainability and reduce complexity.
Consider this cleaner approach:
$(BINDIR)/yq:
$(eval DARCH :=$(ARCH))
ifeq ($(ARCH),aarch64)
$(eval DARCH = $(shell echo arm64))
endif
+ifeq ($(DARCH),x86_64)
+ $(eval DARCH = amd64)
+endif
mkdir -p $(BINDIR)
cd $(TEMP_DIR) \
- && 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)_$(DARCH) -o $(BINDIR)/yq \
&& chmod a+x $(BINDIR)/yqThis change:
- Handles architecture conversion in a consistent way with helm-docs
- Removes nested shell commands
- Simplifies the URL construction
Let's verify the yq release artifacts naming:
Makefile.d/tools.mk (1)
74-78: Verify architecture handling in other installation commands.
While the kubectl installation now correctly handles arm64, let's verify if other tools in the Makefile need similar updates for proper arm64 support.
✅ Verification successful
Architecture handling is already consistent across tool installations
The verification shows that architecture handling is already properly implemented across the Makefile:
- All direct binary downloads (kubectl, telepresence, yq, kind, minikube) consistently handle the aarch64/arm64 conversion
- The Go installation also handles the architecture conversion correctly
- Other tools like kube-linter use OS-only downloads or different installation methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check architecture handling in other tool installation commands
# Look for direct architecture references or download URLs that might need arm64 support
# Search for architecture-related patterns
echo "Searching for architecture-related patterns..."
rg -n "aarch64|arm64|x86_64|amd64|\$(ARCH)" Makefile.d/
# Search for download URLs that might need architecture handling
echo "Searching for download URLs..."
rg -n "curl.*download|wget.*download" Makefile.d/
Length of output: 3293
Makefile.d/k8s.mk (2)
536-539: LGTM: Architecture conversion logic is correct.
The code correctly handles the conversion of aarch64 to arm64 for telepresence installation. This is a common requirement as some tools use different naming conventions for ARM architectures.
536-542: Verify the telepresence installation across architectures.
The changes look good, but it's important to verify that the installation works correctly across different architectures.
✅ Verification successful
Architecture handling in telepresence installation looks correct
The implementation correctly handles the architecture transformations:
- Converts
aarch64toarm64usingDARCHvariable - Transforms
x86_64toamd64usingsubstfunction - Properly handles
arm64without modification - Ensures lowercase conversion for all architectures using
tr '[:upper:]' '[:lower:]'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify telepresence installation URL construction for different architectures
# Test: Check if the URL is correctly constructed for different architectures
echo "Testing URL construction for different architectures:"
echo "aarch64 -> arm64:"
make -n telepresence/install ARCH=aarch64 | grep "curl.*telepresence"
echo "x86_64 -> amd64:"
make -n telepresence/install ARCH=x86_64 | grep "curl.*telepresence"
echo "arm64 (no conversion needed):"
make -n telepresence/install ARCH=arm64 | grep "curl.*telepresence"
Length of output: 703
Script:
#!/bin/bash
# Let's verify the Makefile content directly to analyze the architecture handling
# First, let's look at the telepresence installation target and its dependencies
echo "=== Checking telepresence installation target ==="
cat Makefile.d/k8s.mk | grep -A 10 "telepresence/install:"
# Let's also check if there are any other architecture-related transformations
echo -e "\n=== Checking other architecture transformations ==="
cat Makefile.d/k8s.mk | grep -B 2 -A 2 "DARCH"
Length of output: 1219
Signed-off-by: vankichi <[email protected]>
vankichi
force-pushed
the
bugfix/Makefile/fix-installation-for-arm64
branch
from
f15bfc6
to
b380aae
Compare
Deploying vald with
|
| Latest commit: |
f117d00
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c4ba00d3.vald.pages.dev |
| Branch Preview URL: | https://bugfix-makefile-fix-installa.vald.pages.dev |
Signed-off-by: vankichi <[email protected]>
vankichi
dismissed stale reviews from kpango and coderabbitai[bot]
via
e233512
Signed-off-by: vankichi <[email protected]>
* 🐛 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]>
* 🐛 Fix installation command for arm64 * 🐛 Fix typo * 🐛 Fix --------- Signed-off-by: vankichi <[email protected]> Co-authored-by: Kiichiro YUKAWA <[email protected]>
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