fix: kurtosis install script #2175
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2175 +/- ##
=======================================
Coverage 26.34% 26.34%
=======================================
Files 361 361
Lines 16297 16297
Branches 12 12
=======================================
Hits 4294 4294
Misses 11732 11732
Partials 271 271 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (1)
kurtosis/Makefile (1)
Line range hint
71-73: Remove obsolete .PHONY declarationThe .PHONY list includes
install-kurtosis, but this target has been removed.Apply this diff to update the .PHONY declarations:
.PHONY: start-devnet stop-devnet reset-devnet rm-devnet buildifier-install \ star-lint star-fix start-gcp-devnet-no-build stop-gcp-devnet \ - reset-gcp-devnet rm-gcp-devnet install-kurtosis + reset-gcp-devnet rm-gcp-devnet
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📥 Commits
Reviewing files that changed from the base of the PR and between cafd14e and 564247dcbc9067d86a6031aec876ebc6f4a9fc36.
📒 Files selected for processing (1)
kurtosis/Makefile(3 hunks)
🔇 Additional comments (3)
kurtosis/Makefile (3)
11-11: LGTM: Clean dependency update
The dependency change from install-kurtosis to verify-kurtosis-installed aligns with the project's approach of treating kurtosis as a prerequisite.
20-21: LGTM: Helpful operational documentation
Good addition of documentation explaining the purpose of the --production flag in the context of chaos testing.
7-8: Improved kurtosis detection implementation
The new implementation is more robust and cross-platform compatible. The changes address the Ubuntu detection issue by:
- Using
> /dev/nullinstead of&> /dev/null - Adding quotes around "kurtosis" to prevent word splitting
- Providing clear installation guidance
Let's verify the command works across different shells:
✅ Verification successful
Command verification works consistently across shells
The command command -v "kurtosis" returns expected exit codes across different shells:
- Exit code 1 in bash: Command not found (expected behavior)
- Exit code 127 in sh/dash: Command not found (standard behavior for POSIX shells)
These are the expected behaviors when a command is not found, confirming that the implementation will work reliably across different shells to detect missing kurtosis installation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Test the command in different shells
for shell in bash sh dash; do
echo "Testing in $shell:"
$shell -c 'command -v "kurtosis" > /dev/null; echo $?'
done
Length of output: 381
There are two problems here: 1. On Ubuntu, the "command -v kurtosis &> /dev/null" fails to detect if kurtosis is installed resulting in it trying to install it again. 2. When extracting the kurtosis.tar.gz artifact it generates a kurtosis binary in current folder. However we already have a kurtosis/ folder in our repo so the extraction fails. This commit fixes 1) so it works on any platform and addresses 2) by removing the kurtosis automatic installation which should be fine since its already a prerequisite in our kurtosis/README.md.
fridrik01
force-pushed
the
fix-check-kurtosis
branch
from
564247d to
8bb57c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
kurtosis/Makefile (1)
Line range hint
71-71: Remove 'install-kurtosis' from PHONY targets.The
install-kurtosistarget has been removed, but it's still listed in the.PHONYdeclaration.Remove it from the PHONY targets list:
.PHONY: start-devnet stop-devnet reset-devnet rm-devnet buildifier-install \ star-lint star-fix start-gcp-devnet-no-build stop-gcp-devnet \ - reset-gcp-devnet rm-gcp-devnet install-kurtosis + reset-gcp-devnet rm-gcp-devnet
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📥 Commits
Reviewing files that changed from the base of the PR and between 564247dcbc9067d86a6031aec876ebc6f4a9fc36 and 8bb57c2.
📒 Files selected for processing (1)
kurtosis/Makefile(3 hunks)
🔇 Additional comments (1)
kurtosis/Makefile (1)
11-11: LGTM! Clean dependency change.
The dependency change to verify-kurtosis-installed properly addresses the extraction conflict issue by making kurtosis installation a prerequisite rather than handling it automatically.
| install-kurtosis: | ||
| @echo "Checking for Kurtosis installation..." | ||
| @if ! command -v kurtosis &> /dev/null; then \ | ||
| echo "Kurtosis could not be found, installing..."; \ | ||
| OS=$$(uname -s | tr A-Z a-z); \ | ||
| if [ "$$OS" = "darwin" ]; then \ | ||
| brew install kurtosis-tech/tap/kurtosis-cli; \ | ||
| elif [ "$$OS" = "linux" ]; then \ | ||
| ARCH=$$(uname -m); \ | ||
| if [ "$$ARCH" = "x86_64" ]; then ARCH="amd64"; \ | ||
| elif [ "$$ARCH" = "arm64" ]; then ARCH="arm64"; \ | ||
| else echo "Unsupported architecture $$ARCH for Kurtosis installation" && exit 1; fi; \ | ||
| TAG=`curl -s "https://api.github.com/repos/kurtosis-tech/kurtosis-cli-release-artifacts/releases/latest" | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/'`; \ | ||
| curl -Lo kurtosis.tar.gz "https://github.com/kurtosis-tech/kurtosis-cli-release-artifacts/releases/download/$TAG/kurtosis-cli_${TAG}_${OS}_${ARCH}.tar.gz"; \ | ||
| tar -xzf kurtosis.tar.gz; \ | ||
| rm kurtosis.tar.gz; \ | ||
| chmod +x kurtosis; \ | ||
| sudo mv kurtosis /usr/local/bin/; \ | ||
| else \ | ||
| echo "Unsupported OS $$OS for Kurtosis installation" && exit 1; \ | ||
| fi; \ | ||
| else \ | ||
| echo "Kurtosis is already installed"; \ | ||
| fi |
There was a problem hiding this comment.
I mentioned during planning today about being fine with fixing the Ubuntu compatibility issue and keeping this install code. I changed my mind as this blob of code is complex and difficult to test and guarantee it works on different platforms. I think we should defer the installation to the user as a prerequisite, its already mentioned in the kurtosis/README.
|
Partly replaced with #2470. |
There are two problems here:
On Ubuntu, the
command -v kurtosis &> /dev/nullfails to detect if kurtosis is installed resulting in it trying to install it again.When extracting the
kurtosis.tar.gzartifact it generates akurtosisbinary in current folder. However we already have akurtosis/folder in our repo so the extraction fails.This commit fixes 1) so it works on any platform and addresses 2) by removing the kurtosis automatic installation which should be fine since its already a prerequisite in our
kurtosis/README.md.Summary by CodeRabbit
New Features
Bug Fixes
Documentation