refactor: modularize setup and teardown scripts for resilience#527
Conversation
Restructured both scripts so each step runs as an independent function. If one step fails, subsequent steps still execute. A summary at the end reports all failed/skipped steps. Changes: - Removed `set -e` to prevent early exit on errors - Extracted 6 setup steps: load env, check deps, install deps, setup Neon, start Electric, write .env - Extracted 4 teardown steps: load env, check deps, stop Electric, delete Neon - Added failure tracking with summary report at script end - Script exits non-zero only after all steps complete if any failed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe setup and teardown scripts are refactored from monolithic flows into modular, step-based architectures with structured error tracking. Both scripts now execute sequential steps (environment loading, dependency checking, service configuration), accumulate failures in arrays, and emit consolidated summary reports at completion. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 (2)
.superset/setup.sh (2)
250-258: Consider using an underscore to indicate an intentionally unused loop variable.The static analysis tool flags
ias unused (SC2034). While this is a false positive for a retry loop, you can silence it by using_as the loop variable.🔎 Suggested fix
- for i in {1..30}; do + for _ in {1..30}; do
328-366:step_skippedhelper is defined but never used.The
step_skippedfunction andSKIPPED_STEPSarray are defined but never populated. Consider either removing them if not needed, or implementing skip logic for steps that depend on failed prerequisites.For example, you could skip dependent steps when prerequisites fail:
# Step 5: Start Electric SQL + if [ -z "${DIRECT_URL:-}" ]; then + step_skipped "Start Electric SQL (Neon branch not available)" + elif ! step_start_electric; then - if ! step_start_electric; then step_failed "Start Electric SQL" fi
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.superset/setup.sh.superset/teardown.sh
🧰 Additional context used
🧬 Code graph analysis (1)
.superset/teardown.sh (1)
.superset/setup.sh (5)
error(14-14)success(15-15)warn(16-16)step_load_env(60-80)step_check_dependencies(85-115)
🪛 Shellcheck (0.11.0)
.superset/setup.sh
[warning] 252-252: i appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy API
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (13)
.superset/setup.sh (7)
1-16: LGTM! Well-structured initialization with clear color definitions and helper functions.The removal of
set -ein favor ofset -uo pipefailaligns with the PR objective of continuing execution after step failures while still catching unset variables and pipeline errors.
28-55: LGTM! Clean summary implementation with proper exit code handling.The
print_summaryfunction correctly reports both skipped and failed steps, and returns non-zero only when failures occurred via the[ ${#FAILED_STEPS[@]} -eq 0 ]idiom.
60-80: LGTM! Proper environment loading with appropriate guards.The function validates both the path variable and file existence before sourcing, and correctly uses
set -ato auto-export variables.
85-115: LGTM! Good approach to aggregate all missing dependencies before reporting.This provides better UX by informing users of all missing tools in a single pass rather than failing on each one sequentially.
120-135: LGTM!The function defensively checks for
bunavailability before attempting installation.
140-202: LGTM! Comprehensive Neon branch setup with idempotent behavior.The function correctly handles both existing and new branches, captures command output for error reporting, and exports the necessary variables for subsequent steps.
277-323: LGTM! Defensive .env writing with conditional variable appending.The function correctly checks each variable before writing to avoid appending empty values, and properly handles the copy operation failure.
.superset/teardown.sh (6)
1-55: LGTM! Consistent structure with setup.sh for step tracking and summary reporting.The initialization, helper functions, and
print_summaryimplementation mirror the setup script, providing a consistent user experience.
60-75: LGTM! Correct behavior for teardown context.Unlike setup which loads from
SUPERSET_ROOT_PATH/.env, teardown correctly loads from the workspace's local.envfile that was created during setup.
80-102: LGTM! Appropriate dependency subset for teardown.The teardown script correctly checks only for
neonctlanddocker, which are the only tools needed for its operations.
107-134: LGTM! Idempotent container cleanup with graceful handling.The function correctly handles both existing and already-removed containers, using
warnfor the latter case while still returning success. This is appropriate teardown behavior.
139-168: LGTM! Idempotent branch deletion with proper validation.The function validates required environment variables before attempting deletion and handles the "already deleted" case gracefully.
173-201: LGTM! Consistent main execution flow with setup.sh.The teardown main function follows the same pattern as setup, executing all steps and accumulating failures before printing the summary.
Same note as setup.sh:
step_skippedis defined but unused. Consider removing it for consistency, or implementing skip logic for dependent steps.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
.superset/setup.shand.superset/teardown.shso each step runs as an independent functionTest plan
setup.sh- all 6 steps passedteardown.sh- all 4 steps passed🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.