fix(setup): use reserved port for Electric container in multi-worktree#1417
Conversation
When SUPERSET_PORT_BASE is set, the Electric container now starts on BASE+9 (the reserved offset) instead of an auto-assigned port. This fixes a mismatch where .env wrote the reserved port but the container ran on a random one.
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Fix all issues with AI agents
In @.superset/setup.sh:
- Around line 282-285: The docker port parsing can break on IPv6 or multi-line
output; update the ELECTRIC_PORT assignment (where ELECTRIC_PORT is set from
docker port "$ELECTRIC_CONTAINER" 3000) to robustly handle multi-line and
bracketed IPv6 addresses by selecting the last non-empty line of the output and
extracting only the trailing numeric port (strip any brackets/hosts), e.g. use a
pipeline that filters out empty lines, picks the final line, and applies a regex
to capture only the digits at the end of the line so ELECTRIC_PORT becomes a
clean numeric port.
🧹 Nitpick comments (1)
.superset/setup.sh (1)
263-274: Port binding logic is correct and addresses the PR objective well.The conditional port assignment properly uses the reserved offset (BASE+9) when
SUPERSET_PORT_BASEis set.Minor nit:
$port_flagis intentionally unquoted on line 274 to allow word splitting. A Bash array would be more robust and avoid ShellCheck SC2086, but since the values are fully controlled numerics, this is fine in practice.Optional: use an array for port_flag
# Use reserved port from SUPERSET_PORT_BASE if available, otherwise auto-assign - local port_flag + local port_flag=() if [ -n "${SUPERSET_PORT_BASE:-}" ]; then ELECTRIC_PORT=$((SUPERSET_PORT_BASE + 9)) - port_flag="-p $ELECTRIC_PORT:3000" + port_flag=(-p "$ELECTRIC_PORT:3000") else - port_flag="-p 3000" + port_flag=(-p 3000) fiThen on the
docker runline:if ! docker run -d \ --name "$ELECTRIC_CONTAINER" \ - $port_flag \ + "${port_flag[@]}" \ -e DATABASE_URL="$DIRECT_URL" \
| # Resolve actual port (needed when auto-assigned) | ||
| if [ -z "${ELECTRIC_PORT:-}" ]; then | ||
| ELECTRIC_PORT=$(docker port "$ELECTRIC_CONTAINER" 3000 | cut -d: -f2) | ||
| fi |
There was a problem hiding this comment.
docker port output parsing may break with IPv6 or multi-line output.
docker port can return multiple lines (IPv4 + IPv6), e.g.:
0.0.0.0:12345
[::]:12345
cut -d: -f2 on the IPv6 line yields :] instead of the port. The multi-line output could also embed a newline in ELECTRIC_PORT.
Suggested fix: extract port reliably
- ELECTRIC_PORT=$(docker port "$ELECTRIC_CONTAINER" 3000 | cut -d: -f2)
+ ELECTRIC_PORT=$(docker port "$ELECTRIC_CONTAINER" 3000 | head -1 | awk -F: '{print $NF}')🤖 Prompt for AI Agents
In @.superset/setup.sh around lines 282 - 285, The docker port parsing can break
on IPv6 or multi-line output; update the ELECTRIC_PORT assignment (where
ELECTRIC_PORT is set from docker port "$ELECTRIC_CONTAINER" 3000) to robustly
handle multi-line and bracketed IPv6 addresses by selecting the last non-empty
line of the output and extracting only the trailing numeric port (strip any
brackets/hosts), e.g. use a pipeline that filters out empty lines, picks the
final line, and applies a regex to capture only the digits at the end of the
line so ELECTRIC_PORT becomes a clean numeric port.
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
1 participant
Summary
SUPERSET_PORT_BASEis set, the Electric container now starts onBASE+9(the reserved offset) instead of an auto-assigned port.envwrote the reserved port but the container ran on a random oneChanges
.superset/setup.sh:step_start_electricchecksSUPERSET_PORT_BASEand uses-p $ELECTRIC_PORT:3000instead of-p 3000. Falls back to auto-assign +docker portlookup when no port base is set.Test Plan
SUPERSET_PORT_BASE— Electric auto-assigns port (unchanged behavior)SUPERSET_PORT_BASE— Electric starts onBASE+9, matching.envSummary by CodeRabbit