Conversation
Add opt-in PUID/PGID/UMASK support for the Docker image via docker/entrypoint.sh. When PUID and PGID are set, the container creates the user/group, chown -Rs /config, then runs qui as that UID/GID (via su-exec). - Add docker/entrypoint.sh with user/group creation and privilege drop - Install su-exec in Dockerfile and ci.Dockerfile - Update docker-compose.yml with PUID/PGID/UMASK documentation - Add Permissions section to README.md with examples Motivation: avoid root-owned /config artifacts and ensure directories and files created by hardlink-mode cross-seeding inherit expected ownership.
WalkthroughAdds an entrypoint script and installs su-exec in images; switches ENTRYPOINT to Changes
Sequence DiagramsequenceDiagram
participant Host as Host
participant Container as Container
participant Entrypoint as /entrypoint.sh
participant App as /usr/local/bin/qui
Container->>Entrypoint: start (env: PUID, PGID, UMASK)
activate Entrypoint
Entrypoint->>Entrypoint: apply UMASK (if provided)
alt PUID & PGID set
Entrypoint->>Host: create group (PGID) if missing
Entrypoint->>Host: create user (PUID) if missing
Entrypoint->>Host: chown /config to PUID:PGID
note right of Entrypoint `#d4f1d4`: re-exec as designated user
Entrypoint->>App: su-exec PUID:PGID /usr/local/bin/qui serve
else PUID/PGID not set
note right of Entrypoint `#f1f1d4`: run as current user (root)
Entrypoint->>App: /usr/local/bin/qui serve
end
deactivate Entrypoint
activate App
App->>App: start server (CMD: serve)
deactivate App
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![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
🧹 Nitpick comments (1)
docker/entrypoint.sh (1)
10-10: Consider handling partial PUID/PGID specification.The current logic requires both
PUIDandPGIDto be set. If only one is provided, the container silently runs as root without warning. Consider adding a check to warn users about this requirement or handle the partial case by deriving the missing value.🔎 Suggested improvement
# If PUID/PGID are set, run as that user +if [ -n "$PUID" ] || [ -n "$PGID" ]; then + if [ -z "$PUID" ] || [ -z "$PGID" ]; then + echo "WARNING: Both PUID and PGID must be set together. Running as root." >&2 + fi +fi + if [ -n "$PUID" ] && [ -n "$PGID" ]; then
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
DockerfileREADME.mdci.Dockerfiledocker-compose.ymldocker/entrypoint.sh
🔇 Additional comments (17)
docker/entrypoint.sh (6)
12-14: Group creation logic is correct.The check properly verifies if a GID exists before attempting to create it, preventing errors when the container is restarted.
17-17: GROUP_NAME extraction could be more robust.If multiple groups somehow have the same GID (unlikely but possible in corrupted systems),
awkwill return the first match. The current implementation is acceptable for typical scenarios.
20-22: User creation is properly configured.The flags
-D(don't assign password),-H(no home directory), and-s /sbin/nologin(no shell) appropriately restrict the created user for a container environment.
25-25: Be aware: recursive chown may be slow on large volumes.The
chown -Roperation runs on every container start, which could delay startup if/configcontains many files or is on slow storage. This is the expected behavior for PUID/PGID support, but users with large config directories should be aware of potential startup delays.Consider documenting this behavior in the README if it's not already mentioned, especially for users with large backup archives or extensive cross-seed configurations.
28-28: Privilege drop with su-exec is correctly implemented.The use of
exec su-exec "$PUID:$PGID" /usr/local/bin/qui "$@"properly:
- Drops privileges to the specified UID/GID
- Replaces the shell process with qui (PID 1 benefits)
- Forwards all arguments correctly
1-7: UMASK application and error handling look good.The script correctly:
- Uses
#!/bin/shfor Alpine compatibility- Enables
set -eto fail fast on errors- Applies UMASK before any file operations if specified
Dockerfile (3)
75-75: Runtime dependency addition is appropriate.Adding
su-execto the runtime dependencies is correct for Alpine Linux. It's a minimal, lightweight alternative togosuthat serves the same purpose of dropping privileges.
82-85: Entrypoint installation is correctly implemented.The script is properly:
- Copied to the root directory for easy reference
- Made executable with
chmod +x- Positioned before the ENTRYPOINT declaration
93-93: ENTRYPOINT change preserves backward compatibility.Switching to
/entrypoint.shwhile keepingCMD ["serve"]ensures:
- Users without PUID/PGID continue running as root (backward compatible)
- The binary remains at
/usr/local/bin/quifor direct invocation if needed- Arguments are properly passed through to the application
docker-compose.yml (1)
13-28: Environment variable documentation is clear and helpful.The added comments effectively:
- Explain what happens when PUID/PGID are set (step-by-step process)
- Provide sensible default examples (1000 for user IDs, 022 for UMASK)
- Include an important warning about the
user:field bypassing the entrypoint- Remain properly commented out for opt-in behavior
ci.Dockerfile (3)
67-67: Runtime dependencies match main Dockerfile.The addition of
su-execis consistent with the main Dockerfile approach.
74-77: Entrypoint installation correctly uses build stage reference.The
COPY --from=go-builder /app/docker/entrypoint.shcorrectly references the build stage where the source code is copied, ensuring the entrypoint script is available in the CI build.
85-85: ENTRYPOINT change is consistent with main Dockerfile.The CI build maintains the same entrypoint behavior as the production Dockerfile.
README.md (4)
14-14: Table of Contents properly updated.The new section is correctly added to the TOC with proper indentation under the Docker section.
166-209: Permissions documentation is comprehensive and well-written.The new section effectively:
- Explains the default root behavior and why PUID/PGID are useful
- Provides a clear step-by-step breakdown of what the entrypoint does
- Includes practical examples for both Docker Compose and Docker Run
- Explains UMASK values with common use cases (022, 002, 077)
- Prominently warns about the
user:/--userbypass behavior- Connects the feature to the PR's motivation (cross-seed hardlink ownership)
The documentation quality is excellent and will help users understand and configure this feature correctly.
177-181: UMASK explanation is accurate and helpful.The UMASK value explanations are correct:
022- rw-r--r-- for files, rwxr-xr-x for dirs (standard)002- group-writable for shared environments077- owner-only for maximum privacy
208-208: Important warning about user: bypass is well-placed.The note clearly explains that using Docker's native
user:field or--userflag bypasses the entrypoint's ownership correction logic, directing users to usePUID/PGIDinstead. This prevents a common misconfiguration.
s0up4200
changed the title
|
This would be |
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
4 participants
Summary
docker/entrypoint.shchown -Rs/config, then runs qui as that UID/GID (via su-exec)user:/--userbypass behavior)Motivation: avoid root-owned
/configartifacts and ensure directories/files created by hardlink-mode cross-seeding inherit expected ownership.Test plan
/config/qui.dband logs are owned by the configured UID/GIDSummary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.