Fix #1933: Improve Docker Compose configuration#1971
Open
rishabhvaish wants to merge 4 commits intosteve-community:masterfrom
Open
Fix #1933: Improve Docker Compose configuration#1971rishabhvaish wants to merge 4 commits intosteve-community:masterfrom
rishabhvaish wants to merge 4 commits intosteve-community:masterfrom
Conversation
- Upgrade compose version to 3.8 for better feature support - Add environment variables for better configuration management - Add health checks for both database and application services - Add resource limits to prevent excessive memory usage - Use dedicated Docker network for service isolation - Add proper volume management for database persistence - Add container names for easier management - Improve volume mounts to only include necessary files - Add configurable ports through environment variables - Use service health conditions for proper startup ordering Addresses issue steve-community#1933 by making the Docker setup more production-ready and easier to configure.
- Provides template for environment variables - Documents all configurable options - Includes security notes about password changes - Makes Docker setup more user-friendly and configurable Related to issue steve-community#1933 improvements.
Review Summary by Qodo
WalkthroughsDescription• Upgrade Docker Compose to version 3.8 with production-ready features • Add health checks for database and application services • Implement resource limits and dedicated network isolation • Add environment variable configuration for flexibility • Create .env.example template for easy setup Diagramflowchart LR
A["docker-compose.yml<br/>v3.0"] -->|"Upgrade to v3.8"| B["Enhanced Configuration"]
B -->|"Add health checks"| C["Service Reliability"]
B -->|"Add resource limits"| D["Production Ready"]
B -->|"Add networking"| E["Service Isolation"]
B -->|"Env variables"| F["Flexible Setup"]
G[".env.example<br/>New File"] -->|"Template"| F
File Changes1. docker-compose.yml
|
Code Review by Qodo
1. app uses build: only
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
docker-compose.yml
Outdated
Comment on lines
+46
to
+52
| app: | ||
| container_name: steve-app | ||
| restart: unless-stopped | ||
| build: . | ||
| links: | ||
| - "db:mariadb" | ||
| build: | ||
| context: . | ||
| dockerfile: Dockerfile | ||
| environment: |
There was a problem hiding this comment.
1. app uses build: only 📎 Requirement gap ⛯ Reliability
The app service is configured to build locally (via build:) and does not specify an image:, so the compose setup cannot run purely from a prebuilt image. This violates the requirement to use a prebuilt application image for usability/consistency.
Agent Prompt
## Issue description
The `app` service uses `build:` and does not specify an `image:`, which makes the compose setup depend on a local build.
## Issue Context
Compliance requires `docker-compose.yml` to reference a prebuilt application image so users can start the stack without building locally.
## Fix Focus Areas
- docker-compose.yml[46-52]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Acknowledged. The original compose also uses build-only (no image: tag). Since steve does not currently publish a prebuilt Docker image to any registry, keeping build: is consistent with the project setup. If a published image becomes available in the future, adding an image: field would be a good follow-up.
docker-compose.yml
Outdated
Comment on lines
+22
to
+23
| ports: | ||
| - 3306:3306 | ||
| - "${DB_PORT:-3306}:3306" |
There was a problem hiding this comment.
2. Mariadb port published 📎 Requirement gap ⛨ Security
The db service publishes container port 3306 to the host via a ports: mapping, increasing exposure without an explicit requirement shown in the compose changes. This violates the requirement to avoid publishing MariaDB to the host unless explicitly required.
Agent Prompt
## Issue description
The MariaDB service publishes `3306` to the host, which is disallowed unless explicitly required.
## Issue Context
The application can reach the database via the internal Docker network; host port exposure should be removed or made an explicit opt-in.
## Fix Focus Areas
- docker-compose.yml[22-23]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Good point! Removed the host port mapping for MariaDB in c29fa01. The app connects via the internal Docker network, so there is no need to expose 3306 to the host.
docker-compose.yml
Outdated
Comment on lines
+52
to
+54
| environment: | ||
| - DB_HOST=db | ||
| - DB_PORT=3306 |
There was a problem hiding this comment.
3. Db host hardcoded mariadb 🐞 Bug ✓ Correctness
The app container still waits for and connects to host mariadb, but the compose file no longer provides a mariadb hostname/alias (service is db). This can cause the app to hang/fail during dockerize -wait and/or fail DB connections.
Agent Prompt
### Issue description
`docker-compose.yml` now uses the service name `db` (and sets `DB_HOST=db`), but the image startup logic and docker profile configuration still use `mariadb` as the DB hostname. This breaks service discovery because compose is not providing a `mariadb` DNS name.
### Issue Context
- Dockerfile blocks startup on `dockerize -wait tcp://mariadb:3306`.
- `application-docker.properties` uses `db.ip = mariadb`.
- New compose removes the old `links: db:mariadb` behavior and doesn’t define a network alias.
### Fix Focus Areas
- docker-compose.yml[13-33]
- docker-compose.yml[52-55]
- Dockerfile[21-25]
- src/main/resources/application-docker.properties[7-13]
Suggested implementation directions (choose one):
1) **Add a network alias** so `mariadb` resolves again (minimal change): under `db:` -> `networks:` use expanded syntax and add `aliases: ["mariadb"]`.
2) **Make hostname configurable**: change Dockerfile CMD to wait on `${DB_HOST:-db}` and update `application-docker.properties` default `db.ip` to `db` (or to reference `${DB_HOST}` if you also address build-time filtering noted in another finding).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Great catch! Fixed in c29fa01 — added a network alias so the db service is reachable as mariadb, matching what the Dockerfile and application-docker.properties expect.
docker-compose.yml
Outdated
Comment on lines
+24
to
+28
| environment: | ||
| MYSQL_RANDOM_ROOT_PASSWORD: "yes" | ||
| MYSQL_DATABASE: stevedb | ||
| MYSQL_USER: steve | ||
| MYSQL_PASSWORD: changeme | ||
| MYSQL_DATABASE: ${DB_NAME:-stevedb} | ||
| MYSQL_USER: ${DB_USER:-steve} | ||
| MYSQL_PASSWORD: ${DB_PASSWORD:-changeme} |
There was a problem hiding this comment.
4. Db env vars not effective 🐞 Bug ✓ Correctness
Compose and the new .env.example encourage changing DB_PASSWORD/DB_*, but the build uses Maven resource filtering to bake db.* values from application-docker.properties into resources. This makes runtime env-var changes unreliable and can desync app vs DB container credentials when users follow .env.example.
Agent Prompt
### Issue description
`docker-compose.yml` and `.env.example` suggest users can change DB credentials via environment variables, but the Docker build uses Maven resource filtering and reads `application-docker.properties` at build time. This bakes DB settings into the built app, so changing `.env` alone can easily cause the DB container and the app to diverge.
### Issue Context
- Maven resource filtering is enabled for `application.yml` and `application**.properties`.
- Maven reads `application-${envName}.properties` (docker profile -> `application-docker.properties`) during build.
- README currently documents editing `application-docker.properties` for Docker.
- `.env.example` encourages changing only `DB_PASSWORD`, which is likely to break app↔DB auth unless the app is also reconfigured.
### Fix Focus Areas
- docker-compose.yml[24-28]
- docker-compose.yml[52-58]
- .env.example[4-9]
- pom.xml[95-104]
- pom.xml[195-213]
- src/main/resources/application-docker.properties[7-13]
- README.md[110-115]
Suggested implementation directions (pick one cohesive approach):
1) **True runtime env-var configuration (recommended)**
- Adjust Maven resource filtering to avoid replacing Spring `${...}` placeholders (e.g., filter only `@...@` tokens).
- Update `application-docker.properties` to use Spring env placeholders like `db.password=${DB_PASSWORD:changeme}` (and similarly for schema/user/host/port), so compose `.env` changes take effect at runtime.
2) **Keep build-time config, but remove misleading `.env` knobs**
- Remove `DB_NAME/DB_USER/DB_PASSWORD` from the app service env and from `.env.example` (or make `.env.example` explicitly say you must also edit `application-docker.properties`).
- Keep DB container env vars, but document that app config is controlled via `application-docker.properties` during build.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
You are right — since Maven bakes application-docker.properties at build time, the DB_* env vars in compose would not actually reach the app. Fixed in c29fa01 and 99b828f: removed the misleading DB_* vars from the app service and updated .env.example to clearly document that DB credentials are configured in application-docker.properties.
- Add 'mariadb' network alias for db service to maintain hostname compatibility with Dockerfile and application-docker.properties - Remove DB port exposure to host (app connects via internal network) - Remove misleading DB_* env vars from app service since application-docker.properties is baked at Maven build time - Restore original volume mount (.:/code) since build needs write access - Keep meaningful improvements: healthcheck, named volumes, resource limits, dedicated network, configurable app ports
Remove misleading DB_* variables since application-docker.properties is baked at build time. Add clear note about where to configure database credentials.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1933
This PR improves the Docker Compose setup to be more production-ready while maintaining backward compatibility.
Changes
docker-compose.yml
service_healthydependency so the app waits for a healthy DB (replaces thelinks:directive)mariadbfor the DB service to maintain hostname compatibility with the Dockerfile andapplication-docker.propertiesHTTP_PORT,HTTPS_PORT).env.example (new)
application-docker.properties(baked at Maven build time)Usage
The setup now properly waits for database health before starting the app and provides better resource management.