Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces infrastructure changes to enable no-downtime deployments by separating shared services (MongoDB and nginx) from application services, allowing multiple application instances to run simultaneously on separate networks.
Key changes:
- Extracted MongoDB and nginx into dedicated shared compose files (
docker-compose.shared.staging.ymlanddocker-compose.shared.prod.yml) that can run independently - Added health checks to the gateway service to ensure readiness before accepting traffic
- Configured external Docker networks (
cc-staging-shared-networkandcc-prod-shared-network) to enable communication between separately deployed stacks
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
nginx/app.conf.template |
New templated nginx configuration intended to support dynamic service naming, but lacks processing mechanism and has incorrect upstream naming patterns |
docker-compose.yml |
Added health check configuration for gateway service in development environment |
docker-compose.staging.yml |
Removed mongo and nginx services, added external network configuration, added health checks, removed cross-compose depends_on for mongo |
docker-compose.shared.staging.yml |
New shared services file for staging containing mongo and nginx with their own network |
docker-compose.shared.prod.yml |
New shared services file for production containing mongo and nginx with their own network |
docker-compose.prod.yml |
Removed mongo and nginx services, added external network configuration, added health checks, removed cross-compose depends_on for mongo |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| server ${PROJECT_NAME}-auth-1; | ||
| } | ||
|
|
||
| upstream files { | ||
| server ${PROJECT_NAME}-files-1; | ||
| } | ||
|
|
||
| upstream graphql { | ||
| server ${PROJECT_NAME}-gateway-1; | ||
| } | ||
|
|
||
| upstream web { | ||
| server ${PROJECT_NAME}-web-1; |
There was a problem hiding this comment.
The upstream server configuration uses ${PROJECT_NAME}-auth-1, but this naming pattern doesn't match how Docker Compose names services and containers.
In Docker Compose:
- Services are addressed by their service name (e.g.,
auth, not${PROJECT_NAME}-auth-1) - Container names default to
{project}_{service}_{replica}with underscores (e.g.,myproject_auth_1) - Even if
PROJECT_NAMEis defined, the pattern would be${PROJECT_NAME}_auth_1(underscores), not${PROJECT_NAME}-auth-1(hyphens)
Since all services are on the same cc-shared-network (staging) or cc-prod-shared-network (prod), nginx should reference services by their service names directly:
upstream auth {
server auth;
}
The same issue applies to all upstream blocks (files, gateway, web).
| server ${PROJECT_NAME}-auth-1; | |
| } | |
| upstream files { | |
| server ${PROJECT_NAME}-files-1; | |
| } | |
| upstream graphql { | |
| server ${PROJECT_NAME}-gateway-1; | |
| } | |
| upstream web { | |
| server ${PROJECT_NAME}-web-1; | |
| server auth; | |
| } | |
| upstream files { | |
| server files; | |
| } | |
| upstream graphql { | |
| server gateway; | |
| } | |
| upstream web { | |
| server web; |
| nginx: | ||
| build: ./nginx | ||
| volumes: | ||
| - ./nginx:/etc/nginx/conf.d | ||
| environment: | ||
| - VIRTUAL_HOST=dev-clubs.iiit.ac.in | ||
| # - LETSENCRYPT_HOST=clubs.iiit.ac.in | ||
| ports: | ||
| - "9000:80" | ||
| networks: | ||
| - cc-shared-network | ||
| restart: unless-stopped |
There was a problem hiding this comment.
The nginx service in the shared compose file lacks a depends_on directive to ensure the application services (auth, files, gateway, web) are started before nginx. While these services are in a separate compose file, nginx will attempt to proxy to them and should wait for them to be available, or at least document that the main compose stack must be started first.
Consider adding a comment or documentation about the startup order requirement, since cross-compose depends_on doesn't work.
There was a problem hiding this comment.
Documentation can be added
| nginx: | ||
| build: ./nginx | ||
| volumes: | ||
| - ./nginx:/etc/nginx/conf.d | ||
| environment: | ||
| - VIRTUAL_HOST=clubs.iiit.ac.in | ||
| - LETSENCRYPT_HOST=clubs.iiit.ac.in | ||
| ports: | ||
| - "8000:80" | ||
| networks: | ||
| - cc-shared-network | ||
| restart: unless-stopped |
There was a problem hiding this comment.
The nginx service in the shared compose file lacks a depends_on directive to ensure the application services (auth, files, gateway, web) are started before nginx. While these services are in a separate compose file, nginx will attempt to proxy to them and should wait for them to be available, or at least document that the main compose stack must be started first.
Consider adding a comment or documentation about the startup order requirement, since cross-compose depends_on doesn't work.
| upstream auth { | ||
| server ${PROJECT_NAME}-auth-1; | ||
| } | ||
|
|
||
| upstream files { | ||
| server ${PROJECT_NAME}-files-1; | ||
| } | ||
|
|
||
| upstream graphql { | ||
| server ${PROJECT_NAME}-gateway-1; | ||
| } | ||
|
|
||
| upstream web { | ||
| server ${PROJECT_NAME}-web-1; | ||
| } | ||
|
|
||
| js_path "/etc/nginx/conf.d"; | ||
| js_import http.js; | ||
|
|
||
| js_set $gallery_index http.gallery_index; | ||
| js_set $version http.version; | ||
|
|
||
| server { | ||
| listen 80; | ||
| server_name ${VIRTUAL_HOST}; | ||
| client_max_body_size 100M; | ||
|
|
||
| location ~ ^/(login|logout|logoutCallback) { | ||
| proxy_pass http://auth$request_uri; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header Host $host; | ||
| proxy_redirect off; | ||
| } | ||
|
|
||
| location /files { | ||
| proxy_pass http://files$request_uri; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header Host $host; | ||
| proxy_redirect off; | ||
| } | ||
|
|
||
| location /graphql { | ||
| proxy_pass http://graphql$request_uri; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header Host $host; | ||
| proxy_redirect off; | ||
| } | ||
|
|
||
| error_page 404 /custom_404.html; | ||
| location = /custom_404.html { | ||
| root /usr/share/nginx/html; | ||
| internal; | ||
| } | ||
|
|
||
| error_page 500 502 503 504 /custom_50x.html; | ||
| location = /custom_50x.html { | ||
| root /usr/share/nginx/html; | ||
| internal; | ||
| } | ||
|
|
||
| location ~ ^/static/(.*)$ { | ||
| # expires off; | ||
| # expires 1h; | ||
| expires -1; | ||
| root /usr/share/nginx; | ||
| try_files /$1 /others/$1 /$1/ /others/$1/ =404; | ||
| } | ||
|
|
||
| # ignore cache frontend | ||
| location ~* (service-worker\.js)$ { | ||
| add_header 'Cache-Control' 'no-store, no-cache, must-revalidate, proxy-revalidate, max-age=0'; | ||
| expires off; | ||
| proxy_no_cache 1; | ||
| } | ||
|
|
||
| location / { | ||
| proxy_pass http://web$request_uri; | ||
| proxy_http_version 1.1; | ||
| proxy_set_header Upgrade $http_upgrade; | ||
| proxy_set_header Connection 'upgrade'; | ||
| proxy_set_header Host $host; | ||
| proxy_cache_bypass $http_upgrade; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| } | ||
| } |
There was a problem hiding this comment.
This nginx configuration template file is introduced but there's no mechanism to process it and substitute the environment variables (${PROJECT_NAME}, ${VIRTUAL_HOST}).
The nginx Dockerfile doesn't include any envsubst command or entrypoint script to process .template files. Additionally, the shared compose files mount ./nginx:/etc/nginx/conf.d as a volume, which means any files built into the image would be overridden.
You need to either:
- Add an entrypoint script that runs
envsubston the template file to generate the final configuration, or - Process the template during the Docker build step, or
- Use a different approach to handle dynamic configuration
Example entrypoint approach:
#!/bin/sh
envsubst '${PROJECT_NAME} ${VIRTUAL_HOST}' < /etc/nginx/conf.d/app.conf.template > /etc/nginx/conf.d/app.conf
nginx -g 'daemon off;'There was a problem hiding this comment.
This file is to create nginx conf from git hook and conf for direct use is regardless there already (as default file)
|
@bhavberi is this pr elligible to be merged? |
No description provided.