fix(scripts): append database URLs instead of sed replacement#334
fix(scripts): append database URLs instead of sed replacement#334saddlepaddle merged 1 commit intomainfrom
Conversation
Simpler approach - append branch-specific DATABASE_URL values at the end of .env instead of trying to sed replace them in place. This is more robust since it doesn't depend on the exact format of root .env. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughSingle file refactoring of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.superset/setup.sh (1)
14-18: FixSUPERSET_ROOT_PATHhandling:cpcan fail (or hit-u) even though the earlier load is guarded.
Right now Line 72 assumesSUPERSET_ROOT_PATHis set and points to a readable.env, but Line 14’s check doesn’t enforce that for the latercp. Withset -u, an unsetSUPERSET_ROOT_PATHwill hard-fail here.Suggested tweak:
# Load root .env for this script (provides NEON_PROJECT_ID, etc.) -if [ -n "$SUPERSET_ROOT_PATH" ] && [ -f "$SUPERSET_ROOT_PATH/.env" ]; then +SUPERSET_ROOT_PATH="${SUPERSET_ROOT_PATH:-}" +if [ -n "$SUPERSET_ROOT_PATH" ] && [ -f "$SUPERSET_ROOT_PATH/.env" ]; then set -a source "$SUPERSET_ROOT_PATH/.env" set +a fi + +[ -z "$SUPERSET_ROOT_PATH" ] && error "SUPERSET_ROOT_PATH is required and must point to the Superset root" +[ ! -f "$SUPERSET_ROOT_PATH/.env" ] && error "Missing root .env at: $SUPERSET_ROOT_PATH/.env"Also applies to: 72-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.superset/setup.sh(1 hunks)
⏰ 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 Web
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Build
| # Copy root .env and append 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 | ||
| cat >> .env << EOF | ||
|
|
||
| # Workspace Database (Neon Branch) | ||
| NEON_BRANCH_ID=$BRANCH_ID | ||
| DATABASE_URL=$POOLED_URL | ||
| DATABASE_URL_UNPOOLED=$DIRECT_URL | ||
| EOF |
There was a problem hiding this comment.
“Append” may not actually override: duplicated keys in .env are parsed inconsistently across dotenv loaders.
This change intentionally avoids sed-replacement, but now relies on whatever reads .env to pick the last occurrence of DATABASE_URL / DATABASE_URL_UNPOOLED / NEON_BRANCH_ID. Some consumers treat first definition as authoritative, which would silently ignore the appended block.
If you want “append without depending on exact formatting” and deterministic override, consider filtering those keys out before appending:
-# Copy root .env and append branch-specific values
-cp "$SUPERSET_ROOT_PATH/.env" .env
+# Copy root .env, drop any existing workspace DB keys, then append branch-specific values
+grep -vE '^(NEON_BRANCH_ID|DATABASE_URL|DATABASE_URL_UNPOOLED)=' "$SUPERSET_ROOT_PATH/.env" > .env
cat >> .env << EOF📝 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 append 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 | |
| cat >> .env << EOF | |
| # Workspace Database (Neon Branch) | |
| NEON_BRANCH_ID=$BRANCH_ID | |
| DATABASE_URL=$POOLED_URL | |
| DATABASE_URL_UNPOOLED=$DIRECT_URL | |
| EOF | |
| # Copy root .env, drop any existing workspace DB keys, then append branch-specific values | |
| grep -vE '^(NEON_BRANCH_ID|DATABASE_URL|DATABASE_URL_UNPOOLED)=' "$SUPERSET_ROOT_PATH/.env" > .env | |
| cat >> .env << EOF | |
| # Workspace Database (Neon Branch) | |
| NEON_BRANCH_ID=$BRANCH_ID | |
| DATABASE_URL=$POOLED_URL | |
| DATABASE_URL_UNPOOLED=$DIRECT_URL | |
| EOF |
🤖 Prompt for AI Agents
In .superset/setup.sh around lines 71 to 79, appending branch-specific variables
to .env can produce duplicate keys that some dotenv parsers treat as
authoritative on the first occurrence; instead, remove any existing
NEON_BRANCH_ID, DATABASE_URL and DATABASE_URL_UNPOOLED entries from the target
.env before appending so the appended values are the only definitions. Implement
this by creating a temporary file that filters out lines starting with those
keys (preserving other content), then overwrite .env with the filtered result
and append the branch block; ensure you handle edge cases like missing .env and
preserve file permissions.
| cat >> .env << EOF | ||
|
|
||
| # Workspace Database (Neon Branch) | ||
| NEON_BRANCH_ID=$BRANCH_ID | ||
| DATABASE_URL=$POOLED_URL | ||
| DATABASE_URL_UNPOOLED=$DIRECT_URL | ||
| EOF |
There was a problem hiding this comment.
Consider quoting/escaping .env values to avoid edge-case parsing breaks.
If a connection string ever contains characters that some dotenv parsers treat specially (notably whitespace or #), the unquoted KEY=$VALUE form can break.
At minimum, consider:
-DATABASE_URL=$POOLED_URL
-DATABASE_URL_UNPOOLED=$DIRECT_URL
+DATABASE_URL="${POOLED_URL}"
+DATABASE_URL_UNPOOLED="${DIRECT_URL}"📝 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.
| cat >> .env << EOF | |
| # Workspace Database (Neon Branch) | |
| NEON_BRANCH_ID=$BRANCH_ID | |
| DATABASE_URL=$POOLED_URL | |
| DATABASE_URL_UNPOOLED=$DIRECT_URL | |
| EOF | |
| cat >> .env << EOF | |
| # Workspace Database (Neon Branch) | |
| NEON_BRANCH_ID=$BRANCH_ID | |
| DATABASE_URL="${POOLED_URL}" | |
| DATABASE_URL_UNPOOLED="${DIRECT_URL}" | |
| EOF |
🤖 Prompt for AI Agents
In .superset/setup.sh around lines 73-79, the here-doc writes unquoted env
values which can break dotenv parsers if values contain whitespace or #; change
the write to quote and escape values before appending: build safe variables by
escaping any single quotes (e.g. safe_pooled=$(printf "%s" "$POOLED_URL" | sed
"s/'/'\\\\''/g") and safe_direct=$(printf "%s" "$DIRECT_URL" | sed
"s/'/'\\\\''/g")), then write DATABASE_URL='$safe_pooled' and
DATABASE_URL_UNPOOLED='$safe_direct' into .env (or use printf '%s\n'
"DATABASE_URL='$safe_pooled'" "DATABASE_URL_UNPOOLED='$safe_direct'") so values
are wrapped in single quotes and internal quotes are escaped.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
1 participant
Summary
# Workspace Database (Neon Branch)headerTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.