fix: fix dev setup script#30
Conversation
📝 WalkthroughWalkthroughThe change introduces comprehensive Minikube integration into the development setup script, adding OS/architecture detection, Docker and kubectl readiness checks, Minikube installation and lifecycle management functions, and six new environment variables for cluster configuration. The expanded CLI includes new minikube subcommands wired into the command dispatch. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as dev-setup.sh
participant System as OS/Docker/Minikube
participant Kubectl as kubectl
User->>Script: run setup_minikube()
Script->>Script: detect_os() & detect_arch()
Script->>System: check_docker()
alt Docker not installed
Script->>System: install docker
end
Script->>System: check_kubectl()
alt kubectl not installed
Script->>System: install_kubectl()
end
Script->>System: check minikube status
alt Minikube not installed
Script->>System: install_minikube()
end
Script->>System: start_minikube()
Script->>Kubectl: configure_kubectl()
Kubectl-->>Script: kubeconfig updated
Script-->>User: setup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (5)
hack/dev-setup.sh (5)
103-104: Declare and assign separately to avoid masking return values.If
detect_osordetect_archfails, the exit status is masked by thelocaldeclaration, preventing proper error handling.Suggested fix
install_kubectl() { - local os=$(detect_os) - local arch=$(detect_arch) + local os + local arch + os=$(detect_os) + arch=$(detect_arch) echo "📦 Installing kubectl..."
140-141: Declare and assign separately to avoid masking return values.Same issue as
install_kubectl- the exit status from detection functions is masked.Suggested fix
install_minikube() { - local os=$(detect_os) - local arch=$(detect_arch) + local os + local arch + os=$(detect_os) + arch=$(detect_arch) if command -v minikube &> /dev/null; then
207-218: Fragile JSON parsing and masked return value.The status extraction using
grep -oandcuton JSON is brittle and may break with differentminikubeversions or output formats. Additionally, thelocaldeclaration masks the exit status (SC2155).Suggested fix using jq for robust parsing
# Check if cluster already exists and is running if minikube status -p "${MINIKUBE_PROFILE}" &> /dev/null; then - local status=$(minikube status -p "${MINIKUBE_PROFILE}" -o json 2>/dev/null | grep -o '"Host":"[^"]*"' | cut -d'"' -f4) + local status + if command -v jq &> /dev/null; then + status=$(minikube status -p "${MINIKUBE_PROFILE}" -o json 2>/dev/null | jq -r '.Host // empty') + else + status=$(minikube status -p "${MINIKUBE_PROFILE}" 2>/dev/null | grep -E '^host:' | awk '{print $2}') + fi if [ "$status" = "Running" ]; then
258-265: Interactive prompt may break in non-interactive environments.The
read -pcommand requires a TTY and will fail in CI/CD pipelines or when run non-interactively. Consider supporting a--forceor-yflag, or checking if stdin is a terminal.Optional: Add non-interactive support
delete_minikube() { echo "🗑️ Deleting minikube cluster..." if ! minikube status -p "${MINIKUBE_PROFILE}" &> /dev/null; then echo " ⚠️ Cluster '${MINIKUBE_PROFILE}' does not exist" return 0 fi - read -p " Are you sure you want to delete cluster '${MINIKUBE_PROFILE}'? [y/N] " -n 1 -r - echo - if [[ $REPLY =~ ^[Yy]$ ]]; then + if [ "${FORCE_DELETE:-}" = "true" ] || [ ! -t 0 ]; then + # Non-interactive or forced + minikube delete -p "${MINIKUBE_PROFILE}" + echo " ✓ minikube cluster deleted" + else + read -p " Are you sure you want to delete cluster '${MINIKUBE_PROFILE}'? [y/N] " -n 1 -r + echo + if [[ $REPLY =~ ^[Yy]$ ]]; then + minikube delete -p "${MINIKUBE_PROFILE}" + echo " ✓ minikube cluster deleted" + else + echo " Cancelled" + fi + fi - minikube delete -p "${MINIKUBE_PROFILE}" - echo " ✓ minikube cluster deleted" - else - echo " Cancelled" - fi }
539-547: Interactive prompt may fail in non-interactive environments.Similar to the delete function, this
read -pwill fail when stdin is not a TTY. For the "all" command in CI, this could cause unexpected failures.Optional: Default to skip when non-interactive
# Ask about minikube setup echo "────────────────────────────────────────" echo "" - read -p "🔧 Would you like to setup minikube cluster? [y/N] " -n 1 -r - echo - if [[ $REPLY =~ ^[Yy]$ ]]; then + if [ -t 0 ]; then + read -p "🔧 Would you like to setup minikube cluster? [y/N] " -n 1 -r + echo + else + REPLY="n" # Default to skip in non-interactive mode + fi + if [[ $REPLY =~ ^[Yy]$ ]]; then echo "" setup_minikube else
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/dev-setup.sh
🧰 Additional context used
🪛 Shellcheck (0.11.0)
hack/dev-setup.sh
[warning] 103-103: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 104-104: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 140-140: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 141-141: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 208-208: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (9)
hack/dev-setup.sh (9)
23-30: LGTM!The Minikube configuration defaults are reasonable for a development environment, and the parameter expansion syntax correctly allows overriding via environment variables.
32-51: LGTM!The OS detection logic correctly handles Linux, WSL, macOS, and Windows environments with appropriate fallback for unknown systems.
54-66: LGTM!Architecture detection correctly normalizes different naming conventions to consistent
amd64andarm64outputs.
78-91: LGTM!Good practice to verify both Docker installation and daemon status before proceeding with Minikube setup.
237-247: LGTM!Graceful handling when the cluster doesn't exist, avoiding unnecessary errors.
269-292: LGTM!Comprehensive status output with graceful error handling for missing installations or clusters.
295-305: LGTM!Clean implementation for configuring kubectl context.
308-349: LGTM!Well-structured setup flow with helpful user guidance. The tip about alternative drivers when Docker fails is a nice touch.
491-518: LGTM!Clean command dispatch with sensible default (
setup) and clear error messaging for unknown subcommands.
3 participants
fixes #28
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.