-
Notifications
You must be signed in to change notification settings - Fork 3
fix(docker): align electric http port config #61
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR updates ElectricSQL HTTP port configuration to support newer releases that use nested environment variables. The changes add the ELECTRIC__HTTP__PORT variable (with double underscore) alongside the legacy ELECTRIC_HTTP_PORT to ensure compatibility across ElectricSQL versions. In docker-compose.yml, both variables are set to resolve from the same environment value, defaulting to port 3010. The env.example file is updated to include both variants and corrects the ELECTRIC_SERVICE_URL from port 3000 to 3010. Additionally, the outdated Railway deployment documentation is removed since the project now standardizes on Dokploy/docker-compose for deployments. This aligns with the repository's docker-compose.yml anchor pattern (x-required-db-env) that enforces environment variable consistency across services.
Changed Files
| Filename | Score | Overview |
|---|---|---|
| docs/deployment/railway.md | 5/5 | Complete removal of outdated Railway deployment guide - clean documentation cleanup |
| env.example | 4/5 | Adds nested ELECTRIC__HTTP__PORT variable and corrects service URL from port 3000 to 3010 |
| docker-compose.yml | 4/5 | Adds ELECTRIC__HTTP__PORT environment variable for ElectricSQL service compatibility |
Confidence score: 4/5
- This PR is safe to merge with low risk of production issues
- Score reflects solid configuration hygiene but deducted one point for: (1) potential configuration drift from maintaining dual variables without clear deprecation path, (2) the
exposedirective still references only the legacy variable name which could cause confusion, (3) missing validation that downstream services/health checks have been updated to reference port 3010 instead of 3000 - Pay close attention to env.example and docker-compose.yml to verify both variables consistently default to 3010 and that no services still hardcode references to port 3000
Individual issue scores:
-
Configuration drift from dual variables (3/5 correctness score): Maintaining both
ELECTRIC_HTTP_PORTandELECTRIC__HTTP__PORTwithout a documented deprecation timeline or migration guide could lead to users setting only one variable and experiencing unexpected behavior. Theexposedirective still uses${ELECTRIC_HTTP_PORT:-3010}, creating asymmetry. -
Port mismatch risk (4/5 correctness score): While the PR fixes the env.example service URL from 3000 to 3010, there's no evidence in the diff that downstream health checks or service discovery logic have been audited to ensure they no longer reference the old port.
-
Missing version documentation (3/5 correctness score): The PR body mentions "new releases" of ElectricSQL but doesn't specify which version introduced the nested variable format. This makes it difficult for operators to understand when they can safely drop the legacy variable.
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant Compose as docker-compose.yml
participant Postgres as PostgreSQL Container
participant Electric as ElectricSQL Container
participant Server as Server Container
participant Web as Web Container
Dev->>Compose: "docker-compose up"
Compose->>Postgres: "Start with wal_level=logical"
activate Postgres
Postgres->>Postgres: "Initialize database & shadow DB"
Postgres->>Postgres: "Run healthcheck (pg_isready)"
Postgres-->>Compose: "Healthy"
deactivate Postgres
Compose->>Electric: "Start (depends on Postgres healthy)"
activate Electric
Electric->>Electric: "Read ELECTRIC__HTTP__PORT=${ELECTRIC_HTTP_PORT:-3010}"
Electric->>Electric: "Bind HTTP on 0.0.0.0:3010"
Electric->>Electric: "Bind PG_PROXY on 0.0.0.0:5133"
Electric-->>Compose: "Started"
deactivate Electric
Compose->>Server: "Build & start (depends on Postgres & Electric)"
activate Server
Server->>Server: "Wait for Postgres connection"
Server->>Server: "Run migrations (if RUN_DB_MIGRATIONS=1)"
Server->>Electric: "Connect to ELECTRIC_SERVICE_URL=http://openchat-electric:3010"
Server->>Server: "Start API on port 3000"
Server-->>Compose: "Started"
deactivate Server
Compose->>Web: "Build & start (depends on Server)"
activate Web
Web->>Web: "Read NEXT_PUBLIC_ELECTRIC_URL=http://openchat-electric:3010"
Web->>Web: "Start Next.js on port 3001"
Web-->>Compose: "Started"
deactivate Web
Dev->>Web: "Access application"
Web->>Server: "API requests"
Server->>Electric: "Sync operations via port 3010"
Electric->>Postgres: "Logical replication queries"
Postgres-->>Electric: "Data changes"
Electric-->>Server: "Real-time updates"
Server-->>Web: "Response with sync data"
Web-->>Dev: "Render UI"
Additional Comments (1)
-
docker-compose.yml, line 54 (link)logic:
exposedirective references${ELECTRIC_HTTP_PORT:-3010}, not${ELECTRIC__HTTP__PORT:-3010}. If ElectricSQL 1.1.13 only reads the nested variable (line 48) and a future user removes the legacyELECTRIC_HTTP_PORT(line 47), the container port exposure will break.Issue score: 3/5 – medium risk. Recommend aligning this to the canonical variable once you confirm which format ElectricSQL 1.1.13 requires. For now, the legacy variable is still set, so no immediate breakage. Does ElectricSQL 1.1.13 require
ELECTRIC_HTTP_PORT,ELECTRIC__HTTP__PORT, or both? This will determine which variable should drive theexposedirective.
Context used:
- Context from
dashboard- AGENTS.md (source)
3 files reviewed, 2 comments
| ELECTRIC_INSECURE: ${ELECTRIC_INSECURE:-false} | ||
| ELECTRIC_LOG_LEVEL: ${ELECTRIC_LOG_LEVEL:-info} | ||
| ELECTRIC_HTTP_PORT: ${ELECTRIC_HTTP_PORT:-3010} | ||
| ELECTRIC__HTTP__PORT: ${ELECTRIC_HTTP_PORT:-3010} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: both ELECTRIC_HTTP_PORT and ELECTRIC__HTTP__PORT resolve to the same environment variable ${ELECTRIC_HTTP_PORT:-3010}. If a user overrides only ELECTRIC__HTTP__PORT in their .env, the legacy ELECTRIC_HTTP_PORT on line 47 will still default to 3010, potentially causing confusion.
Issue score: 2/5 – low risk in practice (most users set one variable), but the fallback chain isn't symmetric. Consider using separate variable names or documenting the precedence. Should ELECTRIC__HTTP__PORT read from a distinct env var (e.g., ${ELECTRIC__HTTP__PORT:-3010}) to allow independent override, or is the shared fallback intentional?
Summary
Testing