refactor(docker): enhance Docker setup and documentation#1245
refactor(docker): enhance Docker setup and documentation#1245Audionut merged 12 commits intoAudionut:masterfrom
Conversation
- Updated docker-compose.yml for improved service configuration and added health checks. - Refined Dockerfile to ensure proper permissions and environment settings. - Enhanced upload.py to restore built-in data files when using Docker volumes. - Improved documentation for environment variables and volume mounts in docker-gui-wiki-full.md and docker-wiki-full.md. - Added notes on WebUI configuration and best practices for mounting data directories.
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughAdds Docker build/run support and WebUI orchestration: new .dockerignore and .gitattributes, Dockerfile and docker-compose updates, a docker-entrypoint.sh for ownership/privilege handling, data restoration at startup, session-secret caching, frontend config load/error handling, and expanded Docker documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Container as Container (Docker)
participant Entrypoint as docker-entrypoint.sh
participant App as upload.py
participant FS as File System
participant WebUI as WebUI
Container->>Entrypoint: start (env: PUID/PGID, args)
Entrypoint->>FS: ensure data, tmp, session_secret, config dirs exist
Entrypoint->>FS: chown/chmod based on PUID/PGID
alt TARGET_UID set & not zero
Entrypoint->>FS: chmod /root (711)
Entrypoint->>Entrypoint: drop privileges via gosu -> exec App
else
Entrypoint->>App: exec python /Upload-Assistant/upload.py [args]
end
App->>FS: detect hidden /Upload-Assistant/data (volume mount)
alt data hidden by mount
App->>FS: copy missing files from defaults/data -> data (skip caches)
FS-->>App: report restored files/errors
end
App->>FS: ensure data/config.py exists for WebUI if requested
App->>WebUI: start web server (port 5000)
WebUI->>Container: respond to /api/health for HEALTHCHECK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
|
Attempt to address the current documentation and image challenges. New logic handles a fresh start for empty folder structure and would adjust permissions and access. partially resolve #1187 |
- Removed existing defaults directory before creating a new one to ensure a clean setup. - Updated the copy command to maintain the necessary data structure for the application.
- Added ConfigErrorBoundary to catch rendering errors and display a user-friendly message. - Enhanced loading state in config.html to provide feedback during initial load and potential connection issues. - Updated loadConfigOptions function to include retry logic for loading configuration options.
- Updated .dockerignore to exclude Python bytecode and cache directories more effectively. - Improved docker-compose.yml to clarify environment variable usage for user/group permissions. - Modified Dockerfile to include gosu for privilege management and adjusted entrypoint script for better directory ownership handling. - Enhanced upload.py to skip unwanted directories during data restoration and added error handling for directory creation. - Updated documentation in docker-gui-wiki-full.md to reflect changes in environment variable recommendations and directory permissions.
…ion secret handling - Clarified the requirement for `UA_BROWSE_ROOTS` in Docker, emphasizing its necessity for proper file browsing functionality. - Enhanced explanations regarding the handling of `SESSION_SECRET_FILE`, including potential pitfalls with Docker bind-mounts and recommendations for file management. - Updated various documentation files to ensure consistency and clarity in usage instructions for environment variables and paths.
…er setup - Updated docker-compose.yml and entrypoint.sh to improve explanations regarding the handling of SESSION_SECRET_FILE and directory ownership. - Enhanced documentation in docker-gui-wiki-full.md to reflect changes in directory permissions and the behavior of the entrypoint script when PUID/PGID are set. - Emphasized the importance of proper directory structure for session_secret management and the implications of using Docker bind-mounts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docker-compose.yml`:
- Around line 83-85: When running with PUID/PGID and gosu dropping privileges,
auth.py:get_config_dir() relies on Path.home() and can miss the mounted
webui-auth; add XDG_CONFIG_HOME=/root/.config to the docker-compose.yml
environment block (or export XDG_CONFIG_HOME=/root/.config in the entrypoint
before invoking gosu) so the code resolves the config dir under
/root/.config/upload-assistant and the existing volume mapping
(/root/.config/upload-assistant) is used regardless of the container user
database state.
In `@docker-entrypoint.sh`:
- Around line 18-32: The startup fails if SESSION_SECRET_FILE is bind-mounted as
a file at /Upload-Assistant/session_secret because the existing code always runs
mkdir -p on each path; change the loop around the directories (the for ... in
/Upload-Assistant/data /Upload-Assistant/tmp /Upload-Assistant/session_secret
/root/.config/upload-assistant) to detect if a path exists and is a regular file
(e.g., test -f "$dir") and in that case skip mkdir and just chown the file
(using TARGET_UID/TARGET_GID if set), otherwise create the directory with mkdir
-p and then chown; ensure commands ignore failures when chown is unsupported
(keep the existing "|| true" behavior).
In `@docs/docker-gui-wiki-full.md`:
- Line 214: The sentence "If exposing the WebUI to your LAN/WAN, run behind a
reverse proxy with TLS is recommended." is grammatically incorrect; update it to
a correct form such as "If exposing the WebUI to your LAN/WAN, running it behind
a reverse proxy with TLS is recommended." Replace the original line in
docs/docker-gui-wiki-full.md (the WebUI LAN/WAN recommendation sentence) with
this corrected version.
🧹 Nitpick comments (4)
web_ui/auth.py (1)
153-189: Docker bind-mount directory handling is well thought out.The auto-detection-and-recovery for the common Docker pitfall (bind-mount target created as a directory) is a solid UX improvement. The generated secret is consistent with the read-back path (hex-encoded on disk,
bytes.fromhexon read).A few minor static analysis nits from Ruff — all optional:
- Line 123:
# noqa: PLW0603is flagged as unused sincePLW0603isn't enabled in the Ruff config. Can be removed.- Line 176:
return binside thetryblock — Ruff suggests moving it to anelseblock (TRY300).- Line 178:
log.error(...)in theexcept—log.exception(...)would automatically include the traceback (TRY400).♻️ Optional: address Ruff hints
- global _cached_session_secret # noqa: PLW0603 + global _cached_session_secrettry: b = token_bytes(64) p.write_text(b.hex(), encoding="utf-8") with suppress(Exception): os.chmod(p, 0o600) - log.info("Auto-generated session secret at %s", p) - return b except Exception as e: - log.error("failed to create session secret inside directory: %s", e) + log.exception("failed to create session secret inside directory: %s", e) raise OSError( f"SESSION_SECRET_FILE points to a directory ({f}) and " f"we could not create a session_secret file inside it; " f"check permissions or mount a file instead" ) from e + else: + log.info("Auto-generated session secret at %s", p) + return bdocker-compose.yml (1)
86-95: Healthcheck duplicated between Dockerfile and docker-compose.yml.The Dockerfile (lines 86-87) already defines an identical
HEALTHCHECK. The Compose healthcheck takes precedence at runtime, so there's no conflict, but having both means maintenance in two places. This is a minor nit — keeping it in both places is defensible (Compose for orchestrated deployments, Dockerfile for standalonedocker run).Dockerfile (1)
64-72: Permissions are hardcoded to UID 1000 — consider using a build arg.The
chown -R 1000:1000bakes UID 1000 into the image. This works for the common case, but Unraid users (UID 99:100) rely on theo+rXfallback. The setup is functional, but if you ever want first-class multi-UID support without the "others" permission widening, a--build-argfor the UID would be cleaner. Low priority given the current approach works.docs/docker-gui-wiki-full.md (1)
118-146: Unraid example omitswebui-authvolume — intentional but worth a note.The main Compose snippet (line 86) mounts a
webui-authvolume to/root/.config/upload-assistant, but the Unraid snippet usesSESSION_SECRET_FILE=/Upload-Assistant/data/session_secretinstead (relying on thedatavolume). Both are valid approaches, but Unraid users following both sections might wonder whether they need thewebui-authmount. A brief sentence noting the difference (e.g., "This example uses SESSION_SECRET_FILE inside the data mount instead of a separate webui-auth volume") would reduce confusion.
…nd WebUI - Updated docker-entrypoint.sh to recursively fix ownership of user-placed files and ensure proper permissions for directories and files. - Improved error handling and user feedback in server.py for loading configuration files, including warnings for missing or invalid config files. - Added a config warning banner in the WebUI to inform users about configuration issues, enhancing user experience and clarity.
- Added functionality to reload the configuration from disk within the do_the_thing function, ensuring that changes made via the WebUI or manual edits are reflected without restarting the process. - Included error handling to warn users if the configuration reload fails, enhancing robustness and user feedback.
|
Did this not work out? I'm more than happy to push any tweaks like this for this codebase. |
|
@Audionut I was also preparing for a complete overhaul to fix shutdown issues and kill from ui and so scaffolded around 100 unit tests via AI(Some of them, of course, are not of the best quality, but at least they help to understand that nothing is broken) to make it easier to work with regression. |
|
I'll message you at ath so you have a clear direction of my future plans. |
Docker Image Verification — Test Resultsimage build ok locally on ubuntu and windows
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@upload.py`:
- Around line 1286-1299: Reloading should not destructively clear the live
module-level config before validating the new value; change the block that uses
importlib.reload/_cfg_mod and config.clear()/config.update() to first reload
into a temp variable, check that the reloaded module has a valid config
attribute of the expected type (e.g., hasattr(_cfg_mod, "config") and
isinstance(_cfg_mod.config, dict)), and only then call config.clear() and
config.update(_cfg_mod.config); if validation fails or an exception occurs, keep
the existing config untouched and emit the same console.print warning (including
the error info) instead of leaving config empty.
In `@web_ui/server.py`:
- Around line 2439-2453: The current logic treats an empty dict and a load
failure the same because _load_config_from_file returns {} in both cases; update
_load_config_from_file to return None on failure (and adjust its return type
hint) and return {} for a successfully parsed but empty config, then change the
UI check that sets config_warning to use "if user_config is None" (or explicit
"is not None" where appropriate) instead of falsy checks; also update any other
callers of _load_config_from_file to handle None as the error sentinel.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores