fix: align image storage with koel/koel#2479 + allow Apache to follow symlinks#221
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughImage storage mount target moved to /var/www/html/storage/app/public/images across Dockerfile and all compose files; the build creates that directory with www-data ownership. Apache virtual host now allows FollowSymLinks under /var/www/html/public. README and CHANGELOG updated to document the change and legacy upgrade notice. ChangesImage Storage Path Upgrade
sequenceDiagram
participant Dockerfile
participant docker_compose as docker-compose
participant ContainerFilesystem
participant Apache
Dockerfile->>ContainerFilesystem: create /var/www/html/storage/app/public/images + chown www-data
docker_compose->>ContainerFilesystem: mount image_storage -> /var/www/html/storage/app/public/images
ContainerFilesystem->>Apache: serve image files via public/storage symlink
🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Line 108: The Dockerfile declares a new VOLUME including
"/var/www/html/storage/search-indexes" without ensuring the directory exists or
has the correct ownership; pre-create that path and chown it to the runtime user
before the VOLUME instruction (e.g., use the same owner used for the other
writable mounts) so containers won’t get a root-owned directory that blocks
uploads — add a RUN step to mkdir -p "/var/www/html/storage/search-indexes" and
chown it appropriately prior to the VOLUME declaration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a380aa40-40c0-4515-be12-def81159ab5a
📒 Files selected for processing (7)
CHANGELOG.mdDockerfileREADME.mdapache.confdocker-compose.dev.ymldocker-compose.mysql.ymldocker-compose.postgres.yml
Summary
Fixes #220. koel/koel#2479 (v9.3.0) relocated user-uploaded images from
public/img/storage/tostorage/app/public/images/and exposed them via a symlink atpublic/storage/. The koel/docker image and compose files weren't updated to match, so:Options -SymLinksIfOwnerMatchrejected the new symlink —Symbolic link not allowed or link target not accessible: /var/www/html/public/storage(the 403 in the report).image_storagenamed volume was still bound to the old location. The new location (storage/app/public/images) was an ephemeral path inside the container, so it got wiped on each image pull. Users who upgraded 9.2 → 9.3 saw images work until the next container restart, then lost everything.Changes
apache.conf: added a<Directory /var/www/html/public>block withOptions +FollowSymLinks.Dockerfile:VOLUMEline now declares/var/www/html/storage/app/public/images(instead of the oldpublic/img/storage). Anonymous-volume users get persistence at the new location by default.docker-compose.{mysql,postgres,dev}.yml:image_storagevolume now binds to the new path.README.md: updated the volume table.CHANGELOG.md: upgrade note.Migration story
The named volume's contents persist regardless of mount point. If a user just edits their
docker-compose.ymlto change the bind path from/var/www/html/public/img/storage→/var/www/html/storage/app/public/images, their existing image files appear at the new location automatically. No file copying needed.Users who already upgraded to 9.3.x and lost data (the report's case) will need to restore from backup — there's nothing in this PR that can recover ephemeral-container data that was already wiped on a previous pull.
Test plan
docker-compose.postgres.yml, verify images load (no 403, no missing files).Summary by CodeRabbit
Bug Fixes
Documentation
Note