feat: Update celestia-node.sh to use curl instead of wget #1805
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
public/celestia-node.sh (4)
75-77: Consider adding explicit M1/M2 chip detectionWhile the current architecture detection works correctly, considering the PR's focus on Mac compatibility, it might be helpful to add explicit detection for Apple Silicon chips.
case $ARCH in x86_64) ARCH="x86_64" ;; - aarch64|arm64) ARCH="arm64" ;; + aarch64|arm64|arm64e) ARCH="arm64" ;; # arm64e is used by Apple Silicon *) echo "Unsupported architecture: $ARCH. Exiting." | tee -a "$LOGFILE"; exit 1 ;;
90-94: Add download progress indicatorWhile the curl implementation is correct, consider adding the
-#flag to show a progress bar, improving user experience.-if ! curl -L "$URL" -o "celestia-node_$PLATFORM.tar.gz" >> "$LOGFILE" 2>&1; then +if ! curl -L -# "$URL" -o "celestia-node_$PLATFORM.tar.gz" >> "$LOGFILE" 2>&1; then
121-129: Consider adding cleanup trapWhile the cleanup is good, consider adding a trap to clean up temporary files even if the script fails.
+# Add at the beginning of the script, after TEMP_DIR declaration +trap 'rm -rf "$TEMP_DIR"' EXIT
195-195: Enhance completion messageConsider adding the installed version to the completion message for better user feedback.
-echo "Installation complete. You can now use the celestia binary." | tee -a "$LOGFILE" +echo "Installation of Celestia Node $VERSION complete. You can now use the celestia binary." | tee -a "$LOGFILE"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
public/celestia-node.sh(3 hunks)
🔇 Additional comments (4)
public/celestia-node.sh (4)
31-34: LGTM! Improved user input handling
The simplified conditional using bash's native regex matching improves readability while maintaining the same functionality.
82-83: LGTM! Clear OS validation
The OS validation is concise and properly handles both Linux and macOS (Darwin) with appropriate error handling.
105-107: LGTM! Secure checksum verification
The checksum verification process is well-implemented with proper error handling and security checks.
Also applies to: 113-119
174-175: LGTM! Clear error handling
The error handling for invalid installation choices is clear and consistent.
Also applies to: 189-190
public/celestia-node.sh
Outdated
|
|
||
| # Download the tarball | ||
| if ! wget "$URL" >> "$LOGFILE" 2>&1; then | ||
| if ! curl -L "$URL" -o "celestia-node_$PLATFORM.tar.gz" >> "$LOGFILE" 2>&1; then |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent download methods detected between scripts
The verification reveals that celestia-app.sh uses wget while celestia-node.sh uses curl -L for downloading files. This inconsistency should be standardized:
public/celestia-app.sh: Useswgetfor downloading both the main binary and checksumspublic/celestia-node.sh: Usescurl -L -oconsistently for both downloads
🔗 Analysis chain
Verify consistent curl usage
Let's verify that all download operations use curl consistently throughout the script.
Also applies to: 107-107
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining wget usage and verify curl command consistency
# Check for any remaining wget commands
echo "Checking for wget usage:"
rg "wget"
# Check curl command patterns for consistency
echo -e "\nChecking curl command patterns:"
rg "curl.*-[L#].*-o"
Length of output: 1455
jcstein
changed the title
public/celestia-node.sh
Outdated
| @@ -53,8 +52,6 @@ if [ -n "$USER_VERSION" ]; then | |||
| else | |||
| # Fetch the latest release tag from GitHub | |||
| VERSION=$(curl -s "https://api.github.com/repos/celestiaorg/celestia-node/releases/latest" | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/') | |||
|
|
|||
| # Check if VERSION is empty | |||
public/celestia-node.sh
Outdated
| esac | ||
|
|
||
| # Construct the download URL | ||
| PLATFORM="${OS}_${ARCH}" | ||
| URL="https://github.com/celestiaorg/celestia-node/releases/download/$VERSION/celestia-node_$PLATFORM.tar.gz" | ||
|
|
||
| # Check if URL is valid |
There was a problem hiding this comment.
I think this should still make sure the URL is valid before proceeding.
public/celestia-node.sh
Outdated
| mkdir -p "$GOBIN" | ||
| mv "$TEMP_DIR/celestia" "$GOBIN/" | ||
| chmod +x "$GOBIN/celestia" | ||
| echo "Binary moved to $GOBIN" | tee -a "$LOGFILE" | ||
| # Create a symbolic link in the temporary directory |
There was a problem hiding this comment.
why is this section being removed?
public/celestia-node.sh
Outdated
| @@ -184,100 +157,39 @@ echo # move to a new line | |||
| if [ "$HAS_GO" = true ]; then | |||
| case $REPLY in | |||
| 1) | |||
| # Install to GOBIN | |||
There was a problem hiding this comment.
I think comment is useful, but fine with it if there's good reason!
public/celestia-node.sh
Outdated
| echo "export PATH=\$PATH:$GOBIN" | ||
| echo "" | ||
| fi | ||
| echo "You can now run celestia from anywhere (if $GOBIN is in your PATH)." | tee -a "$LOGFILE" |
There was a problem hiding this comment.
This allows anyone to download and run it without having to navigate to the directory it was installed in, if in temp dir. I think we should leave this.
public/celestia-node.sh
Outdated
| ;; | ||
| 2) | ||
| # Install to /usr/local/bin |
public/celestia-node.sh
Outdated
| sudo mv "$TEMP_DIR/celestia" /usr/local/bin/ | ||
| echo "Binary moved to /usr/local/bin" | tee -a "$LOGFILE" | ||
| # Create a symbolic link in the temporary directory |
There was a problem hiding this comment.
and this. unless I'm missing something
public/celestia-node.sh
Outdated
| ;; | ||
| *) | ||
| echo "" |
There was a problem hiding this comment.
It installs in $TEMP_DIR, so the log should remain to say that is what it's doing IMO
public/celestia-node.sh
Outdated
| ;; | ||
| esac | ||
| else | ||
| case $REPLY in | ||
| 1) | ||
| # Install to /usr/local/bin |
There was a problem hiding this comment.
Same with all of these. outside scope of fixing curl
public/celestia-node.sh
Outdated
| ;; | ||
| esac | ||
| fi | ||
|
|
||
| echo "" |
There was a problem hiding this comment.
This is meant to give someone instructions on how to see the help menu and how to interact with the binary off the start. I think it is best to also leave this. Or at least work it into a separate PR so we can get the curl update in.
|
IMO these are the required changes: main...jcs/demo-curl |
This update modifies "celestia-node.sh" to use curl instead of wget as curl is installed by default on new Macs
Resolves #1808
Summary by CodeRabbit
New Features
curlfor fetching resources.Bug Fixes
Documentation
Refactor