fix(scripts): improve workspace setup/teardown reliability#333
Conversation
- Source root .env at start of setup to get NEON_PROJECT_ID - Copy root .env to worktree and override DATABASE_URLs with branch-specific values - Create .envrc instead of symlinking to root - Make setup idempotent by checking for existing Neon branches - Make teardown idempotent by handling already-deleted branches gracefully - Source local .env at start of teardown to get required vars 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughThe changes modify Superset's workspace setup and teardown scripts to implement a branch-reuse strategy for Neon databases. Setup now checks for existing Neon branches matching the workspace name, reuses them if found, or creates new ones, then populates environment variables with pooled and unpooled connection strings. Teardown improves error handling when deleting branches, with explicit logging and failure-resilience messaging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.superset/teardown.sh (1)
28-31: Potential edge case: Missing NEON_BRANCH_ID after .env load.The script correctly validates that NEON_BRANCH_ID exists, but if .env loading fails silently (e.g., file permission issues), the error message may not be clear. Consider logging the source attempt:
# Load local .env if [ -f ".env" ]; then + echo "📋 Loading workspace configuration..." set -a source .env set +a fiThis provides clearer diagnostics if branch deletion fails due to missing configuration.
.superset/setup.sh (1)
47-69: Branch reuse/create logic is sound, but missing validation for jq output.The logic correctly checks for an existing branch and reuses it, or creates a new one. However, line 51 assumes jq returns exactly one result or empty. If multiple branches have the same name (due to past failures or race conditions), the filter selects the first match. Consider validating the jq output:
EXISTING_BRANCH=$(neonctl branches list --project-id "$NEON_PROJECT_ID" --output json | jq -r ".[] | select(.name == \"$WORKSPACE_NAME\") | .id") +if [ $(echo "$NEON_OUTPUT" | jq '. | length') -gt 1 ]; then + error "Multiple Neon branches found with name '$WORKSPACE_NAME'. Manual cleanup required." +fiHowever, this adds complexity. A simpler fix: use
firstto ensure deterministic behavior:-EXISTING_BRANCH=$(neonctl branches list --project-id "$NEON_PROJECT_ID" --output json | jq -r ".[] | select(.name == \"$WORKSPACE_NAME\") | .id") +EXISTING_BRANCH=$(neonctl branches list --project-id "$NEON_PROJECT_ID" --output json | jq -r "first((.[] | select(.name == \"$WORKSPACE_NAME\")) | .id)")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.superset/setup.sh(2 hunks).superset/teardown.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.superset/teardown.sh (1)
.superset/setup.sh (1)
error(8-8)
⏰ 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 API
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Web
- GitHub Check: Deploy Marketing
- GitHub Check: Build
🔇 Additional comments (3)
.superset/teardown.sh (2)
13-19: Environment loading pattern is safe and consistent.Using
set -a/set +ato source .env is correct and aligns with setup.sh. The conditional check for .env existence prevents errors if the file is missing.
36-41: Graceful error handling for idempotent teardown is well-implemented.The script handles the case where the branch is already deleted by catching the neonctl error and displaying a non-fatal warning. This supports the idempotency requirement stated in the PR objectives.
.superset/setup.sh (1)
34-45: No action needed. Thedotenv .envsyntax in the .envrc file is valid according to direnv's official documentation. Bothdotenv .envanddotenv_if_exists .envare supported; the former is a standard form and does not require modification.
| # Load root .env for this script (provides NEON_PROJECT_ID, etc.) | ||
| if [ -n "$SUPERSET_ROOT_PATH" ] && [ -f "$SUPERSET_ROOT_PATH/.env" ]; then | ||
| set -a | ||
| source "$SUPERSET_ROOT_PATH/.env" | ||
| set +a | ||
| fi |
There was a problem hiding this comment.
Root .env loading requires validation of SUPERSET_ROOT_PATH.
The script attempts to source $SUPERSET_ROOT_PATH/.env but does not validate that the path is absolute or that the file exists with correct permissions. If SUPERSET_ROOT_PATH is unset or malformed, the load silently skips, yet the script later assumes variables like NEON_PROJECT_ID are available. Consider adding explicit validation:
# Load root .env for this script (provides NEON_PROJECT_ID, etc.)
-if [ -n "$SUPERSET_ROOT_PATH" ] && [ -f "$SUPERSET_ROOT_PATH/.env" ]; then
+if [ -z "$SUPERSET_ROOT_PATH" ]; then
+ error "SUPERSET_ROOT_PATH environment variable is required"
+fi
+
+if [ ! -f "$SUPERSET_ROOT_PATH/.env" ]; then
+ error "Root .env file not found at $SUPERSET_ROOT_PATH/.env"
+fi
+
+if true; then
set -a
source "$SUPERSET_ROOT_PATH/.env"
set +a
fiThis ensures the root .env is always available and fails fast with a clear error message.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .superset/setup.sh around lines 13 to 18, the script sources
$SUPERSET_ROOT_PATH/.env without validating SUPERSET_ROOT_PATH or the .env file,
which can lead to missing required env vars later; update the script to first
check that SUPERSET_ROOT_PATH is set, that it is an absolute path, that the file
"$SUPERSET_ROOT_PATH/.env" exists and is readable, and if any check fails print
a clear error to stderr and exit non‑zero; only then enable export (set -a),
source the file, and disable export (set +a) so the script fails fast with a
clear message when the root .env is missing or inaccessible.
| # Copy root .env and override with branch-specific values | ||
| cp "$SUPERSET_ROOT_PATH/.env" .env | ||
| sed -i '' "s|^DATABASE_URL=.*|DATABASE_URL=$POOLED_URL|" .env | ||
| sed -i '' "s|^DATABASE_URL_UNPOOLED=.*|DATABASE_URL_UNPOOLED=$DIRECT_URL|" .env | ||
| echo "NEON_BRANCH_ID=$BRANCH_ID" >> .env |
There was a problem hiding this comment.
Critical: sed -i '' is not portable; environment variable values may break sed patterns.
Two issues here:
-
Portability:
sed -i ''(macOS syntax with empty backup suffix) fails on GNU sed (Linux). Usesed -ialone with conditional logic:-sed -i '' "s|^DATABASE_URL=.*|DATABASE_URL=$POOLED_URL|" .env -sed -i '' "s|^DATABASE_URL_UNPOOLED=.*|DATABASE_URL_UNPOOLED=$DIRECT_URL|" .env +if sed -i.bak "s|^DATABASE_URL=.*|DATABASE_URL=$POOLED_URL|" .env && rm -f .env.bak; then :; else + error "Failed to update DATABASE_URL in .env" +fi +if sed -i.bak "s|^DATABASE_URL_UNPOOLED=.*|DATABASE_URL_UNPOOLED=$DIRECT_URL|" .env && rm -f .env.bak; then :; else + error "Failed to update DATABASE_URL_UNPOOLED in .env" +fi
-
Path Injection: If
$POOLED_URLor$DIRECT_URLcontain sed delimiters (e.g.,|or&), the sed command will break. Escape or use a different delimiter:+# Escape special characters in connection strings for sed +POOLED_URL_ESCAPED=$(echo "$POOLED_URL" | sed -e 's/[\/&]/\\&/g') +DIRECT_URL_ESCAPED=$(echo "$DIRECT_URL" | sed -e 's/[\/&]/\\&/g')
Or, use a safer alternative with
awk:-sed -i '' "s|^DATABASE_URL=.*|DATABASE_URL=$POOLED_URL|" .env -sed -i '' "s|^DATABASE_URL_UNPOOLED=.*|DATABASE_URL_UNPOOLED=$DIRECT_URL|" .env +awk -v pooled="$POOLED_URL" -v direct="$DIRECT_URL" ' + /^DATABASE_URL=/ { print "DATABASE_URL=" pooled; next } + /^DATABASE_URL_UNPOOLED=/ { print "DATABASE_URL_UNPOOLED=" direct; next } + { print } +' "$SUPERSET_ROOT_PATH/.env" > .env.tmp && mv .env.tmp .env
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Copy root .env and override with branch-specific values | |
| cp "$SUPERSET_ROOT_PATH/.env" .env | |
| sed -i '' "s|^DATABASE_URL=.*|DATABASE_URL=$POOLED_URL|" .env | |
| sed -i '' "s|^DATABASE_URL_UNPOOLED=.*|DATABASE_URL_UNPOOLED=$DIRECT_URL|" .env | |
| echo "NEON_BRANCH_ID=$BRANCH_ID" >> .env | |
| # Copy root .env and override with branch-specific values | |
| cp "$SUPERSET_ROOT_PATH/.env" .env | |
| awk -v pooled="$POOLED_URL" -v direct="$DIRECT_URL" ' | |
| /^DATABASE_URL=/ { print "DATABASE_URL=" pooled; next } | |
| /^DATABASE_URL_UNPOOLED=/ { print "DATABASE_URL_UNPOOLED=" direct; next } | |
| { print } | |
| ' "$SUPERSET_ROOT_PATH/.env" > .env.tmp && mv .env.tmp .env | |
| echo "NEON_BRANCH_ID=$BRANCH_ID" >> .env |
🤖 Prompt for AI Agents
In .superset/setup.sh around lines 71 to 75, the current use of sed -i '' is
non‑portable and substituting DATABASE_URL/DATABASE_URL_UNPOOLED directly with
unescaped environment variables can break when those variables contain sed
delimiters or special chars; change the script to use a portable approach
(detect platform and use sed -i '' on macOS vs sed -i on Linux) or avoid
in‑place sed entirely by writing to a temp file and moving it over, and perform
safe variable insertion by escaping the replacement values (or using a different
delimiter that is unlikely to occur) or use a robust tool like awk/perl/envsubst
to replace the DATABASE_URL lines so values with |, /, &, etc. are handled
correctly before appending NEON_BRANCH_ID.
| fi | ||
|
|
||
| # Copy root .env and override with branch-specific values | ||
| cp "$SUPERSET_ROOT_PATH/.env" .env |
There was a problem hiding this comment.
Missing validation and error handling for root .env copy.
Line 72 copies the root .env without checking if the source file exists or if the copy succeeds. If SUPERSET_ROOT_PATH is unset or the file doesn't exist, cp will fail silently (partially, due to set -e), but the error message will not be informative. Add explicit checks:
+[ -f "$SUPERSET_ROOT_PATH/.env" ] || error "Source .env not found at $SUPERSET_ROOT_PATH/.env"
cp "$SUPERSET_ROOT_PATH/.env" .env
+[ -f .env ] || error "Failed to copy .env to workspace"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cp "$SUPERSET_ROOT_PATH/.env" .env | |
| [ -f "$SUPERSET_ROOT_PATH/.env" ] || error "Source .env not found at $SUPERSET_ROOT_PATH/.env" | |
| cp "$SUPERSET_ROOT_PATH/.env" .env | |
| [ -f .env ] || error "Failed to copy .env to workspace" |
🤖 Prompt for AI Agents
In .superset/setup.sh around line 72, the script blindly runs cp
"$SUPERSET_ROOT_PATH/.env" .env without validating SUPERSET_ROOT_PATH or the
source file and without explicit error messages; update the script to (1) verify
SUPERSET_ROOT_PATH is set and non-empty, (2) check that
"$SUPERSET_ROOT_PATH/.env" exists and is a regular readable file, (3) perform
the copy and check its exit status, and (4) print clear, actionable error
messages and exit with a non-zero status if any check or the copy fails so
failures are explicit and debuggable.
| sed -i '' "s|^DATABASE_URL_UNPOOLED=.*|DATABASE_URL_UNPOOLED=$DIRECT_URL|" .env | ||
| echo "NEON_BRANCH_ID=$BRANCH_ID" >> .env | ||
|
|
||
| success "Neon branch created: $WORKSPACE_NAME" |
There was a problem hiding this comment.
Inconsistent success message for branch reuse vs. creation.
Line 77 always prints "Neon branch created: $WORKSPACE_NAME" even when an existing branch is reused (line 54). This is misleading. Update the message to reflect the actual action:
+if [ "$EXISTING_BRANCH" != "" ]; then
+ success "Neon branch reused: $WORKSPACE_NAME"
+else
success "Neon branch created: $WORKSPACE_NAME"
+fiOr move the success message into each branch of the if/else block.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| success "Neon branch created: $WORKSPACE_NAME" | |
| if [ "$EXISTING_BRANCH" != "" ]; then | |
| success "Neon branch reused: $WORKSPACE_NAME" | |
| else | |
| success "Neon branch created: $WORKSPACE_NAME" | |
| fi |
1 participant
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.