feat(backup): add custom database backup mode#20
Conversation
|
Warning Review limit reached
More reviews will be available in 33 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR adds custom database backup and restore mode functionality. It extends the backup service configuration to prompt for and persist database mode settings, implements database validation and dump helpers, integrates custom backup into the backup execution, and extends restore to conditionally use custom restore flows for MySQL/MariaDB and PostgreSQL based on backup configuration. ChangesCustom Database Backup and Restore
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🤖 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 `@lib/pasarguard-backup.sh`:
- Around line 499-503: The script currently writes the raw user-supplied
BACKUP_DB_CONTAINER value (which can be a transient container ID) into ENV_FILE;
instead resolve that input to a stable container name before persisting. When
backup_custom_enabled is true and before appending BACKUP_DB_CONTAINER to
ENV_FILE, try to resolve the provided value (backup_db_container) with docker
inspect --format '{{.Name}}' or docker ps lookup, strip any leading slash, and
fall back to the original value only if resolution fails; then write the
resolved stable container name (and BACKUP_DB_TYPE as before) into ENV_FILE so
later docker inspect/docker exec calls use a persistent container name.
- Around line 1151-1156: The credential check for PostgreSQL/TimescaleDB
currently attempts to connect to the "postgres" database which rejects valid
roles that can only access the target DB; update the docker exec psql calls in
the postgresql | timescaledb) branch (and the other occurrence around
validate_custom_backup_credentials()) to connect to -d "$db_name" instead of -d
postgres (or otherwise use the provided DB name variable), ensuring the psql
invocations inside validate_custom_backup_credentials() use "$db_name" so roles
limited to the application DB validate correctly.
In `@lib/pasarguard-restore.sh`:
- Around line 450-451: The restore logic currently prefers
current_backup_db_type/current_backup_db_container at runtime but later
overwrites .env with the archive's BACKUP_DB_TYPE/BACKUP_DB_CONTAINER, causing
future backups to target the wrong DB; update the .env write path (the block
that persists BACKUP_DB_CONTAINER, BACKUP_DB_TYPE and related
DB_*/SQLALCHEMY_DATABASE_URL entries between approx. the custom restore and the
final .env rewrite) to write the actual in-use values instead of the archive
values—use current_backup_db_type or custom_db_type_raw and
current_backup_db_container or custom_container (whichever was chosen at
runtime) when persisting BACKUP_DB_TYPE and BACKUP_DB_CONTAINER so the preserved
.env matches the restore target.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52e950ea-7c87-4a80-a06f-54c91772181e
📒 Files selected for processing (2)
lib/pasarguard-backup.shlib/pasarguard-restore.sh
- Resolve container IDs to stable names before persisting - Validate PostgreSQL credentials against the target database - Preserve current BACKUP_* configuration after restore
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/pasarguard-backup.sh (2)
1135-1145:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
preferred_passwordis reading the wrong positional argument.This helper defines one override password, but
preferred_passwordis assigned from${3:-}instead of${2:-}. Any caller passing(db_type, preferred_password)will silently ignore the override and fall back to.env/root credentials.Suggested fix
resolve_restore_db_password() { local db_type="$1" - local preferred_password="${3:-}" + local preferred_password="${2:-}" if [ -n "$preferred_password" ]; then printf '%s\n' "$preferred_password"🤖 Prompt for 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. In `@lib/pasarguard-backup.sh` around lines 1135 - 1145, In resolve_restore_db_password the preferred_password is incorrectly read from the third positional argument (${3:-}) so callers passing (db_type, preferred_password) are ignored; update the function to take preferred_password from the second positional parameter (${2:-}) and keep the early-return behavior (printf + return 0) when a preferred_password is provided, otherwise call resolve_custom_backup_db_password "$db_type".
1295-1300:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the container-start helper in the backup path.
perform_custom_database_backup()still hard-fails on a stopped container by callingvalidate_custom_backup_container()directly. The file already hasensure_custom_backup_container_running(), so scheduled custom backups will fail after a reboot or manual stop instead of recovering automatically.Suggested fix
- if ! validate_custom_backup_container "$container" "$log_file"; then + if ! ensure_custom_backup_container_running "$container" "$log_file"; then return 1 fi🤖 Prompt for 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. In `@lib/pasarguard-backup.sh` around lines 1295 - 1300, The backup flow in perform_custom_database_backup() should ensure the target container is running instead of hard-failing: call ensure_custom_backup_container_running("$container" "$log_file") before validate_custom_backup_container(...) and if that helper returns non-zero, return 1 (or propagate the failure) so the container is started/recovered automatically; keep the subsequent validate_custom_backup_credentials(...) call unchanged. Reference functions: perform_custom_database_backup, ensure_custom_backup_container_running, validate_custom_backup_container, validate_custom_backup_credentials.
🤖 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 `@lib/pasarguard-backup.sh`:
- Around line 865-870: After resolving the docker name with
resolve_docker_container_name, do not persist an empty BACKUP_DB_CONTAINER; only
call replace_or_append_env_var "BACKUP_DB_CONTAINER" "$container" when container
is non-empty. If the resolution returns empty, skip writing BACKUP_DB_CONTAINER
(and/or remove any existing BACKUP_DB_CONTAINER entry so restore_command won't
read a blank value). Update the logic around enabled/container resolution (the
block using resolve_docker_container_name and replace_or_append_env_var) to
enforce this conditional behavior.
---
Outside diff comments:
In `@lib/pasarguard-backup.sh`:
- Around line 1135-1145: In resolve_restore_db_password the preferred_password
is incorrectly read from the third positional argument (${3:-}) so callers
passing (db_type, preferred_password) are ignored; update the function to take
preferred_password from the second positional parameter (${2:-}) and keep the
early-return behavior (printf + return 0) when a preferred_password is provided,
otherwise call resolve_custom_backup_db_password "$db_type".
- Around line 1295-1300: The backup flow in perform_custom_database_backup()
should ensure the target container is running instead of hard-failing: call
ensure_custom_backup_container_running("$container" "$log_file") before
validate_custom_backup_container(...) and if that helper returns non-zero,
return 1 (or propagate the failure) so the container is started/recovered
automatically; keep the subsequent validate_custom_backup_credentials(...) call
unchanged. Reference functions: perform_custom_database_backup,
ensure_custom_backup_container_running, validate_custom_backup_container,
validate_custom_backup_credentials.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da434aa3-2187-4067-b915-21c2b0396f8f
📒 Files selected for processing (2)
lib/pasarguard-backup.shlib/pasarguard-restore.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/pasarguard-restore.sh
1 participant
سلام 👋
یه قابلیت جدید به بخش بکاپ اضافه کردم که به کاربر اجازه میده علاوه بر دیتابیس اصلی پاسارگارد، از دیتابیسهای دیگه هم بکاپ بگیره.
قبلش فقط دیتابیس دیفالت پاسارگارد رو پوشش میداد، ولی الان اگه دیتابیس روی یه کانتینر جدا باشه هم میشه راحت بهش وصل شد و بکاپ گرفت.
پشتیبانی هم کامل برای اینا اضافه شده:
هیچ چیزی توی حالت قبلی تغییر نکرده و همون روال دیفالت همچنان مثل قبل کار میکنه. این فقط یه حالت اختیاریه برای سناریوهای کاستوم.
Summary by CodeRabbit