Skip to content

fix(scripts): improve workspace setup/teardown reliability #333

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
saddlepaddle merged 1 commit into from
Dec 12, 2025
Merged

fix(scripts): improve workspace setup/teardown reliability #333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 42 additions & 25 deletions .superset/setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ success() { echo -e "${GREEN}✓${NC} $1"; }

echo "🚀 Setting up Superset workspace..."

# 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
Comment on lines +13 to +18
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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
 fi

This 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.


# Check dependencies
command -v bun &> /dev/null || error "Bun not installed. Install from https://bun.sh"
command -v neonctl &> /dev/null || error "Neon CLI not installed. Run: npm install -g neonctl"
Expand All @@ -24,38 +31,48 @@ echo "📥 Installing dependencies..."
bun install
success "Dependencies installed"

# Link direnv config from root repo if it exists
if [ -n "$SUPERSET_ROOT_PATH" ] && [ -f "$SUPERSET_ROOT_PATH/.envrc" ]; then
echo "🔧 Linking .envrc..."
ln -sf "$SUPERSET_ROOT_PATH/.envrc" .envrc
# Create .envrc for direnv
if [ ! -f .envrc ]; then
echo "🔧 Creating .envrc..."
cat > .envrc << 'ENVRC'
#!/usr/bin/env bash
dotenv .env
ENVRC
if command -v direnv &> /dev/null; then
direnv allow
fi
success "direnv configured"
fi

# Create Neon branch for this workspace
echo "🗄️ Creating Neon branch..."
# Create or get Neon branch for this workspace
WORKSPACE_NAME="${SUPERSET_WORKSPACE_NAME:-$(basename "$PWD")}"
NEON_OUTPUT=$(neonctl branches create \
--project-id "$NEON_PROJECT_ID" \
--name "$WORKSPACE_NAME" \
--output json)

# Parse connection strings from create output
BRANCH_ID=$(echo "$NEON_OUTPUT" | jq -r '.branch.id')
DIRECT_URL=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_uri')
POOLER_HOST=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.pooler_host')
PASSWORD=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.password')
ROLE=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.role')
DATABASE=$(echo "$NEON_OUTPUT" | jq -r '.connection_uris[0].connection_parameters.database')
POOLED_URL="postgresql://${ROLE}:${PASSWORD}@${POOLER_HOST}/${DATABASE}?sslmode=require"

cat >> .env << EOF
NEON_BRANCH_ID=$BRANCH_ID
DATABASE_URL=$POOLED_URL
DATABASE_URL_UNPOOLED=$DIRECT_URL
EOF

# Check if branch already exists
EXISTING_BRANCH=$(neonctl branches list --project-id "$NEON_PROJECT_ID" --output json | jq -r ".[] | select(.name == \"$WORKSPACE_NAME\") | .id")

if [ -n "$EXISTING_BRANCH" ]; then
echo "🗄️ Using existing Neon branch..."
BRANCH_ID="$EXISTING_BRANCH"
# Get connection strings for existing branch
DIRECT_URL=$(neonctl connection-string "$EXISTING_BRANCH" --project-id "$NEON_PROJECT_ID")
POOLED_URL=$(neonctl connection-string "$EXISTING_BRANCH" --project-id "$NEON_PROJECT_ID" --pooled)
else
echo "🗄️ Creating Neon branch..."
NEON_OUTPUT=$(neonctl branches create \
--project-id "$NEON_PROJECT_ID" \
--name "$WORKSPACE_NAME" \
--output json)
BRANCH_ID=$(echo "$NEON_OUTPUT" | jq -r '.branch.id')
# Get connection strings for new branch
DIRECT_URL=$(neonctl connection-string "$BRANCH_ID" --project-id "$NEON_PROJECT_ID")
POOLED_URL=$(neonctl connection-string "$BRANCH_ID" --project-id "$NEON_PROJECT_ID" --pooled)
fi

# Copy root .env and override with branch-specific values
cp "$SUPERSET_ROOT_PATH/.env" .env
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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=.*|DATABASE_URL=$POOLED_URL|" .env
sed -i '' "s|^DATABASE_URL_UNPOOLED=.*|DATABASE_URL_UNPOOLED=$DIRECT_URL|" .env
echo "NEON_BRANCH_ID=$BRANCH_ID" >> .env
Comment on lines +71 to +75
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: sed -i '' is not portable; environment variable values may break sed patterns.

Two issues here:

  1. Portability: sed -i '' (macOS syntax with empty backup suffix) fails on GNU sed (Linux). Use sed -i alone 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

  2. Path Injection: If $POOLED_URL or $DIRECT_URL contain 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.

Suggested change
# 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.


success "Neon branch created: $WORKSPACE_NAME"
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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"
+fi

Or 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.

Suggested change
success "Neon branch created: $WORKSPACE_NAME"
if [ "$EXISTING_BRANCH" != "" ]; then
success "Neon branch reused: $WORKSPACE_NAME"
else
success "Neon branch created: $WORKSPACE_NAME"
fi

echo "✨ Done!"
17 changes: 11 additions & 6 deletions .superset/teardown.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,29 @@ success() { echo -e "${GREEN}✓${NC} $1"; }

echo "🧹 Tearing down Superset workspace..."

# Load local .env
if [ -f ".env" ]; then
# shellcheck disable=SC1091
set -a
source .env
set +a
fi

# Check dependencies
command -v neonctl &> /dev/null || error "Neon CLI not installed. Run: npm install -g neonctl"

# Check required environment variables
NEON_PROJECT_ID="${NEON_PROJECT_ID:-}"
[ -z "$NEON_PROJECT_ID" ] && error "NEON_PROJECT_ID environment variable is required"

# Delete Neon branch for this workspace
WORKSPACE_NAME="${SUPERSET_WORKSPACE_NAME:-$(basename "$PWD")}"
if [ -f ".env" ]; then
# shellcheck disable=SC1091
source .env
fi
BRANCH_ID="${NEON_BRANCH_ID:-}"
if [ -z "$BRANCH_ID" ]; then
error "No NEON_BRANCH_ID found in .env; cannot delete branch"
fi

# Delete Neon branch for this workspace
WORKSPACE_NAME="${SUPERSET_WORKSPACE_NAME:-$(basename "$PWD")}"

echo "🗄️ Deleting Neon branch: $WORKSPACE_NAME ($BRANCH_ID)"
if neonctl branches delete "$BRANCH_ID" --project-id "$NEON_PROJECT_ID" --force 2>/dev/null; then
success "Neon branch deleted: $WORKSPACE_NAME"
Expand Down