Create legacy image #3932
Create legacy image #3932
Conversation
📝 WalkthroughWalkthroughAdds a legacy multi-stage Docker build, CI jobs for checking and publishing that legacy image, a new nginx config, several Docker helper scripts (including an environment dumper and permission checks), editorconfig entry for shell scripts, and minor shell/permission adjustments across Docker scripts. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks✅ Passed checks (1 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker/scripts/permissions-check.sh (1)
20-23: Files are not being reassigned ownership.The
chowncommands only target directories (-type d), but files within these directories may also need ownership changes to ensurewww-datacan read/write them. Consider adding correspondingchowncommands for files or removing-type dto cover both.🔎 Proposed fix to also chown files
# Ensure www-data owns necessary directories -find /app/storage -type d \( ! -user "www-data" -o ! -group "www-data" \) -exec chown "www-data":"www-data" {} + -find /app/bootstrap/cache -type d \( ! -user "www-data" -o ! -group "www-data" \) -exec chown "www-data":"www-data" {} + -find /app/public/uploads -type d \( ! -user "www-data" -o ! -group "www-data" \) -exec chown "www-data":"www-data" {} + 2>/dev/null || true -find /app/public/dist -type d \( ! -user "www-data" -o ! -group "www-data" \) -exec chown "www-data":"www-data" {} + 2>/dev/null || true +# Ensure www-data owns necessary directories and files +find /app/storage \( ! -user "www-data" -o ! -group "www-data" \) -exec chown "www-data":"www-data" {} + +find /app/bootstrap/cache \( ! -user "www-data" -o ! -group "www-data" \) -exec chown "www-data":"www-data" {} + +find /app/public/uploads \( ! -user "www-data" -o ! -group "www-data" \) -exec chown "www-data":"www-data" {} + 2>/dev/null || true +find /app/public/dist \( ! -user "www-data" -o ! -group "www-data" \) -exec chown "www-data":"www-data" {} + 2>/dev/null || true
♻️ Duplicate comments (1)
docker/scripts/validate-env.sh (1)
3-3: Pipefail removed (see earlier comment).Same
pipefailremoval pattern as indocker/scripts/create-admin-user.sh. Refer to that review comment for details.
🧹 Nitpick comments (2)
docker/scripts/dump-env.sh (1)
3-3: Consider addingpipefailfor this script.While other scripts in this PR remove
pipefail, this script actually uses complex pipes (line 39) wherepipefailwould be valuable for error detection. If any command in the pipe chain fails, you'd want to know about it.🔎 Suggested fix
-set -euo +set -euo pipefaildocker/scripts/entrypoint.sh (1)
10-15: Consider removing or uncommenting the dump-env logic.This commented block appears to be intentionally disabled. If it's not needed for the legacy image, consider removing it to reduce clutter. If it might be needed later, a comment explaining why it's disabled would be helpful.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.editorconfig.github/workflows/CICD.ymlDockerfile-legacydocker-compose.yamldocker/nginx.confdocker/scripts/create-admin-user.shdocker/scripts/dump-env.shdocker/scripts/entrypoint.shdocker/scripts/permissions-check.shdocker/scripts/validate-env.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-26T11:45:22.453Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:22.453Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
Applied to files:
Dockerfile-legacydocker-compose.yaml.github/workflows/CICD.yml
🪛 Shellcheck (0.11.0)
docker/scripts/dump-env.sh
[warning] 47-47: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
[warning] 117-117: In POSIX sh, $'..' is undefined.
(SC3003)
🔇 Additional comments (17)
.editorconfig (1)
24-26: LGTM! Shell script formatting configuration added.The addition of shell script formatting rules (2-space indentation) is consistent with other language configurations in the file and aligns with the new shell scripts introduced in this PR.
docker-compose.yaml (2)
6-6: LGTM!The commented legacy image line is left for reference without changing active configuration.
392-392:.envconfiguration correctly implemented between services.The
.envfile is appropriately mounted read-only only tolychee_worker, not tolychee_api. Both services load environment variables viaenv_filedirective, but the worker has additional direct file access for processing operations. This separation aligns with the intended pattern: the API accesses secrets (likeDB_PASSWORD) through environment variables, while the worker can read the file directly if needed during background job processing.docker/scripts/validate-env.sh (1)
13-21: Excellent defensive programming with fallback mechanisms.The enhanced variable resolution for
APP_KEY,DB_PASSWORD, andREDIS_PASSWORDprovides robust fallback mechanisms:
- Check dedicated
*_FILEenvironment variable (Docker secrets pattern)- Fall back to
/app/.envfile- Clear error messages if all methods fail
This improves security (supports Docker secrets) and reliability (multiple fallback paths).
Also applies to: 73-91, 100-110
docker/scripts/permissions-check.sh (1)
3-3: Removal ofpipefailis intentional but worth documenting.The
pipefailoption was removed from the shell options. This is acceptable for this script since it doesn't use pipelines that could silently fail. However, consider adding a comment explaining whypipefailis omitted for future maintainers.docker/scripts/entrypoint.sh (4)
56-68: Good defensive check for running processes before usermod.The check for running
www-dataprocesses before attemptingusermod/groupmodprevents potential issues with modifying user IDs while processes are running. The fallback whenusermodis not available is also handled correctly.
17-40: Database readiness check is well-implemented.The logic correctly handles MySQL/PostgreSQL by checking port availability with netcat, with a reasonable timeout (60s) and a small buffer after the port opens for full initialization.
107-111: PHP-FPM startup logic is appropriate for legacy image.In the legacy Dockerfile,
/app/frankenphp_targetwon't exist (only/app/docker_targetis created), so PHP-FPM will correctly be started. This aligns with the nginx-based serving model in the legacy image.
161-201: Worker mode auto-restart loop is well-designed.The implementation includes:
- Graceful shutdown via signal trapping
- Proper exit code capture with
|| EXIT_CODE=$?pattern- Configurable queue parameters via environment variables
- Appropriate warnings for sync driver usage
.github/workflows/CICD.yml (2)
354-365: Legacy build job correctly specifies Dockerfile-legacy.The
docker_legacy_buildjob properly usesfile: Dockerfile-legacyin the build-push action, unlike the check job. The-legacysuffix in metadata is correctly configured.
336-352: Metadata configuration looks correct for legacy tagging.The
suffix=-legacy,onlatest=trueflavor ensures all tags includinglatestget the-legacysuffix, which properly distinguishes legacy images from the main FrankenPHP-based images.docker/nginx.conf (3)
16-31: Smart logging configuration to reduce health check noise.The map-based conditional logging correctly filters out successful (200) health checks from localhost, reducing log noise while preserving all other access logs. The dual logging to file and stdout is appropriate for containerized deployments.
95-98: Good security practice denying direct access to non-index PHP files.Returning 403 for all
.phpfiles exceptindex.phpprevents potential exposure of internal framework files and is a security best practice for Laravel applications.
84-84: Correct socket path version and clarify deployment context.The socket path references
php8.5-fpm.sock, but the Dockerfile usesphp8.4-alpine. More importantly, thisnginx.confis only used in legacy deployment mode—the default deployment uses FrankenPHP directly (viaphp artisan octane:start --server=frankenphp), and the nginx configuration is never loaded.If maintaining legacy PHP-FPM + Nginx support: update the socket path to match the PHP version being used (either downgrade the legacy entrypoint from
php-fpm8.5tophp-fpm8.4, or align the base image to PHP 8.5). Otherwise, consider removing this file if legacy deployment is no longer supported.Dockerfile-legacy (3)
17-27: Composer stage is well-optimized.The use of
--ignore-platform-reqsis acceptable here since platform requirements are validated in the final production image. The cleanup of markdown files and test directories helps reduce image size. Based on learnings, this pattern is approved for Lychee's multi-stage builds.
77-119: Comprehensive package installation with proper cleanup.The installation includes all necessary PHP extensions, image processing tools, and supporting utilities. The ImageMagick PDF policy modification is properly documented, and apt cache is cleaned for image size optimization.
133-146: Storage and permissions setup is correctly configured.The directory structure creation with appropriate ownership and permissions aligns with the permission model enforced by
permissions-check.sh. Thedocker_targetmarker file correctly identifies this as the legacy (non-FrankenPHP) image.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
d7415
left a comment
d7415
left a comment
There was a problem hiding this comment.
I haven't finished reading dump-env but won't have time for that until tomorrow earliest.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
docker-compose.yaml (1)
135-135: Document the SKIP_PERMISSIONS_CHECKS setting.This past review comment remains unaddressed. Setting
SKIP_PERMISSIONS_CHECKS: "yes"skips filesystem permission initialization at container startup. Without documentation, users may copy this configuration without understanding:
- Why permissions are skipped by default
- When they should change this to
"no"- What permission requirements must be met if skipped (www-data ownership, specific chmod values)
Add an inline comment explaining this configuration choice to prevent permission failures in user deployments.
docker/scripts/dump-env.sh (2)
131-131: Fix security issue: .env file permissions are too permissive.Setting permissions to
644makes the.envfile readable by all users on the system. Since this file contains sensitive credentials (DB_PASSWORD, APP_KEY, Redis passwords, OAuth secrets, AWS keys, etc.), it should only be readable by the owner.🔎 Recommended fix
-chmod 644 "$ENV_FILE" +chmod 600 "$ENV_FILE"
47-47: Fix naked redirection for better portability.The redirection lacks a command. While this works in bash, use
: >"$ENV_FILE"ortrue >"$ENV_FILE"for better portability and to resolve the shellcheck warning.🔎 Proposed fix
->"$ENV_FILE" +: >"$ENV_FILE"
🧹 Nitpick comments (2)
docker/scripts/entrypoint.sh (2)
180-189: Consider adjusting timeout relative to max-time.The
--timeout=3600for individual jobs matches the defaultWORKER_MAX_TIME=3600. Having these equal could cause issues where a job times out just as the worker restarts. The job timeout should typically be less than the worker's max-time to allow for graceful job completion before worker restart.🔎 Suggested adjustment
# --tries=3: retry failed jobs up to 3 times - # --timeout=3600: kill job if it runs longer than 1 hour + # --timeout=3500: kill job if it runs longer than ~58 minutes (slightly less than max-time) # --sleep=3: sleep 3 seconds when queue is empty # --max-time=$WORKER_MAX_TIME: restart worker after N seconds (memory leak mitigation) php artisan queue:work \ --queue="$QUEUE_NAMES" \ --tries=3 \ - --timeout=3600 \ + --timeout=3500 \ --sleep=3 \ --max-time="$WORKER_MAX_TIME" || EXIT_CODE=$?This ensures jobs have a buffer to complete before the worker restarts.
111-115: PHP version is correct but could be more maintainable.The hard-coded
php-fpm8.5on line 114 matches the PHP version installed inDockerfile-legacyand only executes in traditional Docker mode (the FrankenPHP build mode uses a separate code path). The version is correct for the current setup, but if PHP versions are updated in the future, bothDockerfile-legacyand this entrypoint script would need manual updates.Making this more dynamic could improve maintainability:
- php-fpm8.5 + php-fpm$(php -r 'echo PHP_MAJOR_VERSION.".".PHP_MINOR_VERSION;')
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Dockerfiledocker-compose.yamldocker/scripts/create-admin-user.shdocker/scripts/dump-env.shdocker/scripts/entrypoint.shdocker/scripts/permissions-check.shdocker/scripts/validate-env.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/scripts/create-admin-user.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:22.453Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
📚 Learning: 2026-01-04T19:57:36.325Z
Learnt from: d7415
Repo: LycheeOrg/Lychee PR: 3932
File: docker/scripts/create-admin-user.sh:3-3
Timestamp: 2026-01-04T19:57:36.325Z
Learning: In docker/scripts/*.sh, enable strict Bash error handling by using 'set -euo pipefail'. This ensures the script exits on error, treats unset variables as errors, and propagates errors through pipes. Apply this to relevant scripts (e.g., create-admin-user.sh) to improve reliability and safety of Docker-related setup tasks.
Applied to files:
docker/scripts/validate-env.shdocker/scripts/dump-env.shdocker/scripts/entrypoint.shdocker/scripts/permissions-check.sh
📚 Learning: 2025-12-26T11:45:18.032Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:18.032Z
Learning: In multi-stage Dockerfiles (as used in Lychee), you may use --ignore-platform-reqs in the composer build stage when generating vendor files, because this stage only affects vendor generation and the final production image is what enforces platform requirements. Ensure: (1) this flag is confined to the build stage and does not affect the production runtime image, (2) the final stage uses the correct platform constraints and installed extensions, and (3) the produced vendor files are compatible with the production image’s PHP/runtime platform. This pattern should be applied to all Dockerfiles that contain a build stage with composer, not just a single file.
Applied to files:
Dockerfile
📚 Learning: 2025-12-26T11:45:22.453Z
Learnt from: ildyria
Repo: LycheeOrg/Lychee PR: 3889
File: Dockerfile:8-23
Timestamp: 2025-12-26T11:45:22.453Z
Learning: In multi-stage Dockerfiles for the Lychee project, using `--ignore-platform-reqs` in the composer build stage is acceptable because it's only used to generate vendor files that are copied to the production stage. The platform requirements that matter are in the final production image, not the intermediate build stage.
Applied to files:
docker-compose.yaml
🪛 Shellcheck (0.11.0)
docker/scripts/dump-env.sh
[warning] 47-47: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: 3️⃣ Legacy Dockerfile Lint
- GitHub Check: 2️⃣ PHP dist / 8.5 - sqlite
- GitHub Check: 2️⃣ PHP dist / 8.4 - sqlite
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Precomputing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Precomputing
- GitHub Check: 1️⃣ PHP tests / 8.5 - postgresql -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Precomputing
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Unit
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - sqlite -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- ImageProcessing
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Webshop
- GitHub Check: 1️⃣ PHP tests / 8.4 - mariadb -- Install
- GitHub Check: 1️⃣ PHP tests / 8.4 - sqlite -- Install
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Feature_v2
- GitHub Check: 1️⃣ PHP tests / 8.5 - mariadb -- Unit
🔇 Additional comments (19)
Dockerfile (1)
87-87: LGTM!Adding
bashto support the bash-based scripts introduced across the repository (entrypoint.sh, permissions-check.sh, validate-env.sh, etc.) is appropriate and necessary.docker/scripts/permissions-check.sh (1)
1-38: LGTM!The script correctly implements:
- Bash shebang with strict error handling (
set -euo pipefail) as per learnings- Conditional execution via SKIP_PERMISSIONS_CHECKS
- Restrictive permissions (750 for sensitive directories, 755 for public, 640/644 for files)
- Proper ownership assignment to www-data
Based on learnings, the error handling approach is appropriate for Docker-related setup tasks.
docker-compose.yaml (2)
397-398: LGTM!Making the worker depend on the API service with a health check condition ensures proper startup ordering and prevents the worker from attempting to process jobs before the API is ready.
392-392: The explicit .env volume binding is necessary for the worker service.The
env_filedirective exports environment variables, but does not create a physical file. Thevalidate-env.shscript contains fallback logic that directly reads/app/.envas a file (usinggrepto extractAPP_KEY,DB_PASSWORD, andREDIS_PASSWORD). The explicit volume mount at line 392 is required for this fallback mechanism to work. These are not redundant configurations; they serve complementary purposes.docker/scripts/validate-env.sh (1)
1-137: LGTM!The reformatting improves readability while preserving all functional logic:
- Bash shebang with strict error handling (
set -euo pipefail) as per learnings- Proper fallback mechanisms for secrets (APP_KEY_FILE, DB_PASSWORD_FILE, REDIS_PASSWORD_FILE)
- Clear validation of required environment variables
- Appropriate security warnings for production configurations
The restructured conditionals and case statements make the validation flow easier to follow.
docker/scripts/dump-env.sh (1)
1-137: Overall approach is well-structured.The script implements a sophisticated filtering mechanism:
- Scans Laravel config files to identify legitimate environment variables
- Excludes system/infrastructure variables (PATH, DOCKER_*, etc.)
- Properly escapes special characters and handles complex values
- Uses strict error handling (
set -euo pipefail) as per learningsOnce the two issues above are addressed, this will be a robust environment management solution.
Based on learnings, the error handling approach is appropriate for Docker-related setup tasks.
docker/scripts/entrypoint.sh (13)
1-3: LGTM! Strict error handling is properly configured.The script correctly uses
set -euo pipefailfor robust error handling, ensuring it exits on errors, treats unset variables as errors, and propagates errors through pipes.
5-8: LGTM! Clean startup flow with environment validation.
10-19: Commented code remains for testing purposes.This section was previously discussed. As noted, it's intentionally commented while testing if FrankenPHP can function without explicit .env file generation.
21-44: LGTM! Robust database readiness check.The connection wait logic properly handles timeouts and provides clear feedback. The additional 2-second delay after port availability is a pragmatic approach to ensure the database is fully initialized.
46-58: LGTM! Proper UID/GID validation with safe defaults.The default values (33 for www-data) and range validation (33-65534) are appropriate, preventing privilege escalation while maintaining flexibility.
60-74: LGTM! Safe user modification with process check.The
pgrepcheck prevents modifying www-data while it has running processes, which is essential for avoiding runtime issues.
78-80: LGTM! Cache cleanup is appropriate for container startup.
82-89: LGTM! Helpful misconfiguration guard with documentation link.This check prevents a common Docker volume mounting mistake and provides clear guidance for resolution.
95-109: LGTM! Proper Laravel optimization sequence for web mode.The migration and caching steps follow Laravel best practices for production deployment.
120-170: LGTM! Comprehensive worker initialization and configuration.The migration wait logic, queue configuration, and signal handling are well-implemented with appropriate timeouts and clear messaging.
191-205: LGTM! Robust restart logic with proper exit code handling.The auto-restart loop correctly captures exit codes and respects shutdown signals for graceful termination.
207-210: LGTM! Clear error handling for invalid mode.
76-76: permissions-check.sh is properly available in the Docker image.The script exists in
docker/scripts/and is correctly COPY'd to/usr/local/bin/in the Dockerfile with explicit chmod +x permissions during the Docker build. The call at line 76 of entrypoint.sh will execute successfully in containers.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.