feat: Optimize Dockerfile #3193
Conversation
|
Warning Rate limit exceeded@transparentChange has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a new lightweight app Dockerfile, introduces/updates docker-compose configurations to build the app locally and define db/smtp services, adds a healthcheck in the Docker image, updates an env file with a new variable, and changes a script shebang from bash to sh. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 0
🔭 Outside diff range comments (1)
docker/app/Dockerfile (1)
34-35: Health-check will fail unlesswget(or curl) is added to the image
eclipse-temurin:21-alpinedoes not ship withwget, yet the health-check relies on it. The container will therefore exit with1on start-up.-RUN apk --no-cache add libxml2 bash +RUN apk --no-cache add libxml2 wget bash
🧹 Nitpick comments (2)
docker/app/Dockerfile (1)
3-3: Slim the image by dropping the unusedbashdependency
cmd.shnow runs under/bin/sh; nothing in this image requires Bash any longer. Removing it saves ±8 MB.-RUN apk --no-cache add libxml2 wget bash +RUN apk --no-cache add libxml2 wgetdocker/app/cmd.sh (1)
1-10: Harden the start-up script (set -eu) for safer error handlingA silent failure (e.g., missing
java) will leave the container running with an exit code 0. Add strict mode at the top:-#!/bin/sh +#!/bin/sh +set -euBusyBox
shsupports-eand-u;pipefailis not available, so keep it minimal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker/.env(1 hunks)docker/app/Dockerfile(1 hunks)docker/app/cmd.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: laz-001
PR: tolgee/tolgee-platform#0
File: :0-0
Timestamp: 2025-05-09T01:35:14.224Z
Learning: Changes to PostgreSQL container names, ports, Liquibase changelog paths, and test data directories in the Tolgee platform are often made to provide dedicated resources to parallel test groups (like `ee-test:test` and `server-app:test`), preventing resource congestion and collisions that could lead to deadlocks during testing.
📚 Learning: 2025-05-09T01:35:14.224Z
Learnt from: laz-001
PR: tolgee/tolgee-platform#0
File: :0-0
Timestamp: 2025-05-09T01:35:14.224Z
Learning: Changes to PostgreSQL container names, ports, Liquibase changelog paths, and test data directories in the Tolgee platform are often made to provide dedicated resources to parallel test groups (like `ee-test:test` and `server-app:test`), preventing resource congestion and collisions that could lead to deadlocks during testing.
Applied to files:
docker/.envdocker/app/Dockerfile
📚 Learning: 2025-05-09T01:30:41.011Z
Learnt from: laz-001
PR: tolgee/tolgee-platform#3061
File: ee/backend/tests/src/test/resources/application.yaml:41-41
Timestamp: 2025-05-09T01:30:41.011Z
Learning: In the Tolgee platform, the `data-path` property in application.yaml is handled declaratively by the Spring Boot framework, which automatically creates and manages the specified directory without requiring explicit setup/teardown code.
Applied to files:
docker/.env
🪛 dotenv-linter (3.3.0)
docker/.env
[warning] 5-5: [IncorrectDelimiter] The spring.datasource.password key has incorrect delimiter
[warning] 5-5: [LowercaseKey] The spring.datasource.password key should be in uppercase
[warning] 5-5: [UnorderedKey] The spring.datasource.password key should go before the spring.datasource.url key
[warning] 6-6: [EndingBlankLine] No blank line at the end of the file
[warning] 6-6: [UnorderedKey] The TOLGEE_POSTGRES_AUTOSTART_ENABLED key should go before the exposePort key
🔇 Additional comments (1)
docker/.env (1)
5-5:spring.datasource.passwordis not a valid environment variable name
Dots are illegal in variable names consumed by Docker Compose ([A-Za-z_][A-Za-z0-9_]*). The key will be silently ignored, resulting in the password defaulting to an empty string.Consider rewriting the DB settings with uppercase, underscore-separated names, e.g.:
-spring.datasource.password=postgres +SPRING_DATASOURCE_PASSWORD=postgres…and updating the application to read the new name (Spring Boot already maps underscores ⇢ dots).
|
@JanCizmar @Anty0 Requesting your review |
|
@transparentChange Hi! Thanks for the PR! We discussed the possibility of a similar change internally. Unfortunately, removing the bundled Postgres instance wouldn't be straightforward, as users already rely on it. Implementing this change would require some manual migration steps for them. One idea would be to create a separate Docker image without the bundled Postgres, while keeping the existing one with the bundled Postgres. New self-hosters could then use the new image, and we could keep the old one for existing deployments. |
|
@transparentChange However, we can create separate "lite" image without postgres. But I would not change the default |
|
This PR is stale because it has been open for 30 days with no activity. |
transparentChange
force-pushed
the
transparentChange/docker
branch
3 times, most recently
from
6c0be33 to
248b7fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
docker/app/Dockerfile.lite (3)
19-24: Set WORKDIR and ensure file permissions/ownership are consistent.Without WORKDIR, some scripts relying on relative paths may fail. Also prefer non-root ownership on copied assets.
Apply this diff:
+# Set working directory early +WORKDIR /app @@ -COPY BOOT-INF/lib /app/lib -COPY META-INF /app/META-INF -COPY BOOT-INF/classes /app -COPY --chmod=755 cmd.sh /app +COPY BOOT-INF/lib /app/lib +COPY META-INF /app/META-INF +COPY BOOT-INF/classes /app +COPY --chmod=755 cmd.sh /appIf you add a non-root user (see next comment), also use
--chown=app:appon the COPY lines.
12-14: Run as non-root and pre-create/chown data dir.Drop root to reduce blast radius; make sure /data exists and is writable.
Apply this diff:
VOLUME /data + +# Create non-root user and ensure ownership +RUN addgroup -S app && adduser -S -G app app \ + && mkdir -p /data \ + && chown -R app:app /app /data @@ -ENTRYPOINT ["/app/cmd.sh"] +USER app +ENTRYPOINT ["/app/cmd.sh"]Optionally add
--chown=app:appto the COPY instructions above.Also applies to: 29-31
3-3: Trim unnecessary packages.If
bashandlibxml2aren’t strictly required, omit them to keep the “lite” image small.-RUN apk --no-cache add libxml2 bash +RUN apk --no-cache add curl ca-certificates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker-compose.yml(1 hunks)docker/app/Dockerfile.lite(1 hunks)docker/docker-compose.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docker-compose.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-09T01:35:14.224Z
Learnt from: laz-001
PR: tolgee/tolgee-platform#0
File: :0-0
Timestamp: 2025-05-09T01:35:14.224Z
Learning: Changes to PostgreSQL container names, ports, Liquibase changelog paths, and test data directories in the Tolgee platform are often made to provide dedicated resources to parallel test groups (like `ee-test:test` and `server-app:test`), preventing resource congestion and collisions that could lead to deadlocks during testing.
Applied to files:
docker/docker-compose.yml
🔇 Additional comments (2)
docker/app/Dockerfile.lite (2)
19-23: Build-context sanity check for BOOT-INF/META-INF paths.These paths must exist relative to the Docker build context. Verify the build pipeline produces them where this Dockerfile expects them.
Would you like me to add a compose target or CI job to fail fast if these paths are missing?
29-31: ENTRYPOINT path looks good.Assuming /app/cmd.sh exists after COPY and has +x, this is correct.
Note:
COPY --chmod=755requires BuildKit; confirm your build environment enables it.
| # Environment variables for configuration | ||
| ENV HEALTHCHECK_PORT=8080 \ | ||
| spring_profiles_active=docker |
There was a problem hiding this comment.
Use the canonical Spring Boot env var name.
Spring reads SPRING_PROFILES_ACTIVE; lower-case spring_profiles_active won’t be picked up by relaxed binding.
Apply this diff:
-ENV HEALTHCHECK_PORT=8080 \
- spring_profiles_active=docker
+ENV HEALTHCHECK_PORT=8080 \
+ SPRING_PROFILES_ACTIVE=docker📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Environment variables for configuration | |
| ENV HEALTHCHECK_PORT=8080 \ | |
| spring_profiles_active=docker | |
| # Environment variables for configuration | |
| ENV HEALTHCHECK_PORT=8080 \ | |
| SPRING_PROFILES_ACTIVE=docker |
🤖 Prompt for AI Agents
In docker/app/Dockerfile.lite around lines 15 to 17, the environment variable
uses lower-case spring_profiles_active which Spring Boot won’t pick up; replace
it with the canonical upper-case SPRING_PROFILES_ACTIVE and keep the value set
to docker (e.g. ENV HEALTHCHECK_PORT=8080 \ SPRING_PROFILES_ACTIVE=docker) so
Spring recognizes the active profile.
| # Health check to ensure the app is up and running | ||
| HEALTHCHECK --interval=10s --timeout=3s --retries=20 \ | ||
| CMD wget --spider -q "http://127.0.0.1:$HEALTHCHECK_PORT/actuator/health" || exit 1 |
There was a problem hiding this comment.
Healthcheck may fail on Alpine due to wget availability/flags; prefer curl.
Alpine’s base often has BusyBox wget with limited flags (e.g., --spider may be unsupported). Install curl and use it in HEALTHCHECK to avoid false negatives.
Apply this diff:
-RUN apk --no-cache add libxml2 bash
+RUN apk --no-cache add curl ca-certificates
@@
-HEALTHCHECK --interval=10s --timeout=3s --retries=20 \
- CMD wget --spider -q "http://127.0.0.1:$HEALTHCHECK_PORT/actuator/health" || exit 1
+HEALTHCHECK --interval=10s --timeout=3s --retries=20 \
+ CMD curl -fsS "http://127.0.0.1:${HEALTHCHECK_PORT}/actuator/health" >/dev/null || exit 1Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docker/app/Dockerfile.lite around lines 33 to 35, the HEALTHCHECK uses wget
with flags that may not exist in Alpine's BusyBox wget causing false failures;
replace the wget-based check with a curl-based check (e.g., curl --fail --silent
--show-error http://127.0.0.1:$HEALTHCHECK_PORT/actuator/health || exit 1) and
ensure curl is installed earlier in the Dockerfile (add apk add --no-cache curl
where packages are installed or in the build stage). Make the HEALTHCHECK use
curl and confirm the package installation occurs before the HEALTHCHECK line.
docker/docker-compose.yml
Outdated
| build: | ||
| context: ../build/docker | ||
| dockerfile: Dockerfile | ||
| ports: | ||
| - ${exposePort}:8080 |
There was a problem hiding this comment.
🧩 Analysis chain
Validate env and paths used by the build.
- If you do keep a build here, ensure the Dockerfile path and context are correct (the new
Dockerfile.litelives atdocker/app/Dockerfile.lite, not in../build/docker). - Confirm
${exposePort}is defined; otherwise port mapping will fail.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for exposePort in .env files..."
fd -a -HI .env | xargs -r rg -n '^exposePort=' -C0 || echo "exposePort not found"
echo "Listing candidate Dockerfiles and build contexts..."
fd -a -HI Dockerfile -E 'node_modules' -E 'build' -E 'dist'
fd -a -HI Dockerfile.lite -E 'node_modules' -E 'build' -E 'dist'Length of output: 318
Fix docker-compose build context & dockerfile; exposePort is defined (8091)
- In docker/docker-compose.yml change build.context from ../build/docker to ./app and build.dockerfile from Dockerfile to Dockerfile.lite (docker/app contains Dockerfile and Dockerfile.lite).
- .env (docker/.env) defines exposePort=8091 so the port mapping is present.
🤖 Prompt for AI Agents
In docker/docker-compose.yml around lines 9 to 13 the build context and
dockerfile are incorrect and should point at the app directory and the lite
Dockerfile: change build.context from ../build/docker to ./app and
build.dockerfile from Dockerfile to Dockerfile.lite; leave the ports mapping
as-is since docker/.env already defines exposePort=8091.
docker/docker-compose.yml
Outdated
| build: | ||
| context: ../build/docker | ||
| dockerfile: Dockerfile |
There was a problem hiding this comment.
Don’t replace the default image with a local build; add an alternate profile/override.
Maintainers requested keeping tolgee/tolgee as the default. Move the build of the new variant to a separate compose file or profile to avoid breaking existing users.
Apply this diff to keep the default here:
- app:
- build:
- context: ../build/docker
- dockerfile: Dockerfile
+ app:
+ image: tolgee/tolgeeThen add a docker/docker-compose.lite.yml (or a profile lite) that builds the lite image (pointing to docker/app/Dockerfile.lite) for new self-hosters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| build: | |
| context: ../build/docker | |
| dockerfile: Dockerfile | |
| app: | |
| image: tolgee/tolgee |
🤖 Prompt for AI Agents
In docker/docker-compose.yml around lines 9-11 the service was changed to build
from ../build/docker Dockerfile which replaces the default tolgee/tolgee image;
revert this so the default image remains tolgee/tolgee in this file, and instead
add an alternate compose file (docker/docker-compose.lite.yml) or a compose
profile named "lite" that defines the build for the lite variant pointing to
docker/app/Dockerfile.lite; keep docker/docker-compose.yml using the original
image reference, create docker/docker-compose.lite.yml (or add a profile block)
that sets build.context and build.dockerfile to the lite Dockerfile and document
usage (docker-compose -f docker-compose.yml -f docker-compose.lite.yml up
--build or docker-compose --profile lite up) so existing users are not affected.
7410f60 to
1bab3bc
Compare
transparentChange
force-pushed
the
transparentChange/docker
branch
from
1bab3bc to
dfe0234
Compare
|
@JanCizmar Thank you for the review. Like you suggested I added the new Dockerfile.lite file and a comment for how it can be hooked up to the docker-compose. |
3 participants
Changes the Dockerfile to use a base image without postgresql. Instead, the external postgres is used when running with docker-compose. This change significantly reduces build size and performance.
docker images local output:
Summary by CodeRabbit
New Features
Chores