Skip to content

Conversation

@m4dm4rtig4n
Copy link
Contributor

Summary

  • Create dedicated docker-compose.conductor.yml for Conductor with only frontend and backend
  • Remove static container_name declarations to support concurrent instances on same host
  • Move port definitions to docker-compose.override.yml for dynamic port allocation
  • Configure external Redis/PostgreSQL access via host.docker.internal inside containers

Test plan

  • Run ./scripts/conductor-start.sh - starts only frontend and backend (no Redis/PostgreSQL)
  • Verify ports are dynamically allocated (8001, 8082, etc.)
  • Run ./scripts/conductor-stop.sh - cleanly stops services
  • Multiple concurrent instances can run on different ports without conflicts

🤖 Generated with Claude Code

Create a dedicated docker-compose.conductor.yml for Conductor that only runs
frontend and backend services, using external Redis and PostgreSQL. Remove
static container names to avoid conflicts between instances, and move port
definitions to docker-compose.override.yml for dynamic port allocation.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Copilot AI review requested due to automatic review settings December 5, 2025 22:55
@m4dm4rtig4n m4dm4rtig4n merged commit d93523b into main Dec 5, 2025
8 checks passed
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements multi-instance deployment support for MyElectricalData's Conductor mode by decoupling frontend/backend services from infrastructure dependencies (Redis/PostgreSQL) and enabling dynamic port allocation. The changes allow multiple development instances to run concurrently on the same host without port conflicts or container name collisions.

Key changes:

  • Created docker-compose.conductor.yml for lightweight frontend/backend-only deployments with external service dependencies
  • Removed static container_name declarations and hardcoded ports from docker-compose.yml to prevent instance conflicts
  • Enhanced conductor-start.sh to generate dynamic port mappings and convert localhost URLs to host.docker.internal for container-to-host communication

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
docker-compose.conductor.yml New Conductor-specific compose file with frontend and backend services only, configured for external Redis/PostgreSQL
docker-compose.yml Removed static container names and hardcoded ports; added host.docker.internal mapping; removed unused caddy volumes
scripts/conductor-start.sh Added URL transformation logic (localhost→host.docker.internal), dynamic override file generation, and updated docker compose commands
scripts/conductor-stop.sh Updated to use conductor-specific compose files with fallback logic
Comments suppressed due to low confidence (1)

scripts/conductor-start.sh:108

  • [nitpick] The sed command uses -i.bak which creates backup files (.env.api.bak) that are later removed on line 108. However, macOS requires -i '' (with a space) while Linux accepts -i.bak. This current approach works but leaves the sed command less portable.

Consider using a more portable approach:

sed "s|^REDIS_URL=.*|REDIS_URL=$REDIS_URL_DOCKER|" .env.api > .env.api.tmp && mv .env.api.tmp .env.api

Or use -i'' (without space) which is more compatible across platforms:

sed -i'' "s|^REDIS_URL=.*|REDIS_URL=$REDIS_URL_DOCKER|" .env.api
        sed -i.bak "s|^REDIS_URL=.*|REDIS_URL=$REDIS_URL_DOCKER|" .env.api
    else
        echo "REDIS_URL=$REDIS_URL_DOCKER" >> .env.api
    fi

    # Mettre à jour DATABASE_URL (version Docker)
    if grep -q "^DATABASE_URL=" .env.api; then
        sed -i.bak "s|^DATABASE_URL=.*|DATABASE_URL=$DATABASE_URL_DOCKER|" .env.api
    else
        echo "DATABASE_URL=$DATABASE_URL_DOCKER" >> .env.api
    fi

    # Mettre à jour FRONTEND_URL et BACKEND_URL pour le dev
    sed -i.bak "s|^FRONTEND_URL=.*|FRONTEND_URL=http://localhost:$FRONTEND_PORT|" .env.api
    sed -i.bak "s|^BACKEND_URL=.*|BACKEND_URL=http://localhost:$BACKEND_PORT|" .env.api

    rm -f .env.api.bak

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +5 to +52
services:

# Backend FastAPI
backend:
build:
context: ./apps/api
dockerfile: Dockerfile
restart: unless-stopped
env_file:
- ./.env.api
environment:
- PYTHONUNBUFFERED=1
- WATCHFILES_FORCE_POLLING=true
- WATCHFILES_POLL_INTERVAL=1000
- TZ=Europe/Paris
extra_hosts:
- "host.docker.internal:host-gateway"
volumes:
- ./apps/api/data:/app/data
- ./apps/api/src:/app/src
- ./apps/api/static:/app/static
- backend_logs:/logs
command: ["uvicorn", "src.main:app", "--host", "0.0.0.0", "--port", "8000", "--reload", "--reload-dir", "/app/src"]
healthcheck:
test: ["CMD", "python", "-c", "import urllib.request; urllib.request.urlopen('http://localhost:8000/ping')"]
interval: 30s
timeout: 10s
retries: 3
start_period: 40s

# Frontend React/Vite
frontend:
build:
context: ./apps/web
dockerfile: Dockerfile.dev
restart: unless-stopped
volumes:
- ./apps/web/src:/app/src
- ./apps/web/public:/app/public
- ./apps/web/index.html:/app/index.html
- ./apps/web/vite.config.ts:/app/vite.config.ts
- ./apps/web/tsconfig.json:/app/tsconfig.json
- ./apps/web/tsconfig.node.json:/app/tsconfig.node.json
- ./apps/web/tailwind.config.js:/app/tailwind.config.js
- ./apps/web/postcss.config.js:/app/postcss.config.js
- /app/node_modules
depends_on:
- backend
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

Missing network definitions in docker-compose.conductor.yml. The services should define which networks they belong to, similar to docker-compose.yml where services use networks: - myelectricaldata. Without network definitions, the backend and frontend services won't be able to communicate with each other properly.

Add network definitions to both services:

  backend:
    # ... existing config ...
    networks:
      - myelectricaldata

  frontend:
    # ... existing config ...
    networks:
      - myelectricaldata

networks:
  myelectricaldata:
    driver: bridge

Copilot uses AI. Check for mistakes.

# Arrêter les conteneurs
docker compose down
# Arrêter les conteneurs (utiliser docker-compose.conductor.yml + override)
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The comment states "Arrêter les conteneurs" (Stop the containers) but the line was modified to include additional information about using docker-compose.conductor.yml + override. The comment should be updated to accurately reflect the change:

# Arrêter les conteneurs (docker-compose.conductor.yml + override avec fallback)
Suggested change
# Arrêter les conteneurs (utiliser docker-compose.conductor.yml + override)
# Arrêter les conteneurs (docker-compose.conductor.yml + override avec fallback)

Copilot uses AI. Check for mistakes.
services:
backend:
depends_on: []
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

[nitpick] Setting depends_on: [] overrides the dependency configuration but may not work as expected in all Docker Compose versions. The empty array syntax is valid YAML, but some older versions of Docker Compose might not properly handle this override pattern.

Consider being more explicit with a comment explaining the override, or verify this works with the minimum Docker Compose version required by the project:

  backend:
    # Override: no service dependencies in Conductor mode (uses external Redis/PostgreSQL)
    depends_on: []
Suggested change
depends_on: []

Copilot uses AI. Check for mistakes.
Comment on lines +75 to +76
log_info "Redis (host): $REDIS_URL"
log_info "Redis (docker): $REDIS_URL_DOCKER"
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The log_info calls for REDIS_URL will print the full Redis connection URL, which in many deployments includes an authentication password (e.g., redis://:password@host:6379/0). If logs are accessible to other users or systems, this can leak live Redis credentials and allow unauthorized access to cached data or session stores. Consider either omitting the Redis URL from logs or masking credentials similarly to DATABASE_URL (e.g., strip or redact the password portion before logging).

Suggested change
log_info "Redis (host): $REDIS_URL"
log_info "Redis (docker): $REDIS_URL_DOCKER"
log_info "Redis (host): $(echo $REDIS_URL | sed 's#redis://:\([^@]*\)@#redis://:***@#')"
log_info "Redis (docker): $(echo $REDIS_URL_DOCKER | sed 's#redis://:\([^@]*\)@#redis://:***@#')"

Copilot uses AI. Check for mistakes.
@m4dm4rtig4n m4dm4rtig4n deleted the fix-container-name-conflict branch December 18, 2025 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants