Add scripts and docs to configure docker backups #2534
Conversation
this is no longer needed as we are using .rst file format
- Add relative path - Move docker backup docs to backup.rst
|
I've changed the code and the docs as requested, are there more changes? |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce new environment variables for backup configuration in the Changes
Poem
Warning Rate limit exceeded@dumbstertruck3 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 13 seconds before requesting another review. ⌛ 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (7)
scripts/backup.sh (2)
7-8: That comment on line 7 seems a bit... redundant, doesn't it?Also, we should validate the database name:
date=$(date +%Y%m%d%H%M%S) -#name the file +# Validate database name +if [[ -z "${POSTGRES_DB}" ]]; then + echo "Error: POSTGRES_DB environment variable is not set" >&2 + exit 1 +fi + backup_file="${POSTGRES_DB}_backup_${date}.dump"
10-11: The cleanup could be a bit more informative.Let's add validation and logging:
+# Validate retention period +if [[ -z "${DB_BACKUP_RETENTION_PERIOD}" ]] || ! [[ "${DB_BACKUP_RETENTION_PERIOD}" =~ ^[0-9]+$ ]]; then + echo "Error: DB_BACKUP_RETENTION_PERIOD must be a positive integer" >&2 + exit 1 +fi + # Remove old backup/backups -docker exec -t ${container_name} find "/backups" -name "${POSTGRES_DB}_backup_*.dump" -type f -mtime +${DB_BACKUP_RETENTION_PERIOD} -exec rm {} \; +docker exec -t ${container_name} find "/backups" -name "${POSTGRES_DB}_backup_*.dump" -type f -mtime +${DB_BACKUP_RETENTION_PERIOD} -print -exec rm {} \; | \ + sed 's/^/Removing old backup: /'docker-compose.yaml (1)
14-14: Consider documenting backup directory permissions.While the volume mount is fine, it might be nice if we documented the recommended permissions for
${BACKUP_DIR:-./care-backups}to prevent unauthorized access to potentially sensitive backup data.Consider adding a note in the documentation about setting appropriate permissions:
+ # Ensure backup directory has restricted permissions + mkdir -p ./care-backups + chmod 700 ./care-backupsREADME.md (1)
75-75: A comma would make this sentence flow better, wouldn't it?The sentence would be more polished with a comma after "restore":
-For backup and restore use [this](/docs/databases/backup.rst) documentation. +For backup and restore, use [this](/docs/databases/backup.rst) documentation.🧰 Tools
🪛 LanguageTool
[uncategorized] ~75-~75: Possible missing comma found.
Context: ...re/pkgs/container/care) For backup and restore use this ...(AI_HYDRA_LEO_MISSING_COMMA)
docs/databases/backup.rst (3)
4-4: Consider using a more robust path referenceThe relative path
../../scripts/backup.shmight break if the documentation structure changes. Perhaps we could use a more... resilient approach?Consider using a repository-relative path or documenting the expected directory structure.
12-12: The explanation paragraph could use some structureThis rather lengthy paragraph could benefit from some formatting improvements. I mean, if you want users to actually read it...
Consider breaking it into bullet points:
-The script automates the process of creating PostgreSQL database backups from a Docker container. It generates a backup file(``.dump``) using the pg_dump utility in PostgreSQL and stores these files in the path configured in ``$BACKUP_DIR`` environment variable which is binded to ``/backups`` in the docker container. Backup files older than ``$DB_BACKUP_RETENTION_PERIOD`` days are deleted when the script is executed by default it is set to 7 days. The backup file is saved with the name ``care_backup_%Y%m%d%H%M%S.sql``. +The script automates PostgreSQL database backups in Docker: + +* Generates backup files using ``pg_dump`` utility +* Stores files in ``$BACKUP_DIR`` (mounted as ``/backups`` in container) +* Automatically removes backups older than ``$DB_BACKUP_RETENTION_PERIOD`` days (default: 7) +* Names backups using format: ``care_backup_%Y%m%d%H%M%S.sql``
107-107: Consider a more professional closing noteWhile the friendly tone is appreciated, perhaps we could phrase this more professionally?
- There are way easier ways to do this. If anyone has any particular idea, feel free to make a PR :) + Note: Contributions to improve this process are welcome. Please submit a pull request with any enhancements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.env.example(1 hunks)CONTRIBUTING.md(2 hunks)README.md(1 hunks)docker-compose.yaml(1 hunks)docs/databases/backup.rst(1 hunks)scripts/backup.sh(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .env.example
- CONTRIBUTING.md
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~75-~75: Possible missing comma found.
Context: ...re/pkgs/container/care) For backup and restore use this ...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (3)
scripts/backup.sh (1)
1-15: One tiny detail we might want to check...
We should verify the backup directory exists in the container before proceeding:
docker-compose.yaml (1)
14-14: Verify backup script compatibility with the mount point.
The volume mount looks mostly correct, but let's make absolutely sure the backup script is looking for /backups and not /backup.
✅ Verification successful
The mount point is perfectly aligned with the backup script. How... convenient.
The backup script at scripts/backup.sh explicitly uses /backups (with an 's') for all its operations:
- Finding and removing old backups:
/backups - Writing new backup files:
/backups/${backup_file}
So the volume mount path in docker-compose.yaml is exactly right. Not that we had any doubts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if backup script uses the correct mount point
# Search for backup path references
rg -i '/backup' --type sh
Length of output: 361
README.md (1)
75-75: Verify the backup documentation link
Let's ensure the documentation link is valid and accessible.
✅ Verification successful
The documentation link appears to be perfectly valid
The backup documentation not only exists but also contains relevant content about database backup and restore procedures. I suppose we can all appreciate when documentation links actually work as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the backup documentation file exists
if [ -f "docs/databases/backup.rst" ]; then
echo "✓ Documentation file exists"
# Check if the file has content
if [ -s "docs/databases/backup.rst" ]; then
echo "✓ Documentation file has content"
else
echo "✗ Documentation file is empty"
fi
else
echo "✗ Documentation file not found at expected path"
fi
Length of output: 217
Script:
#!/bin/bash
# Let's check the content of the backup documentation to ensure it's actually useful
cat docs/databases/backup.rst | head -n 5
Length of output: 274
🧰 Tools
🪛 LanguageTool
[uncategorized] ~75-~75: Possible missing comma found.
Context: ...re/pkgs/container/care) For backup and restore use this ...
(AI_HYDRA_LEO_MISSING_COMMA)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2534 +/- ##
===========================================
- Coverage 70.03% 69.20% -0.83%
===========================================
Files 198 211 +13
Lines 11674 11944 +270
Branches 1607 1208 -399
===========================================
+ Hits 8176 8266 +90
- Misses 3107 3302 +195
+ Partials 391 376 -15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
scripts/backup.sh (2)
1-9: Perhaps we could validate all required environment variables upfront?While the script does validate the
.envfile existence, it might be slightly more elegant to validate all required environment variables at the start:source "${ENV_FILE}" + +# Validate required environment variables +required_vars=("POSTGRES_USER" "POSTGRES_DB" "DB_BACKUP_RETENTION_PERIOD") +for var in "${required_vars[@]}"; do + if [[ -z "${!var:-}" ]]; then + echo "Error: $var environment variable is not set" >&2 + exit 1 + fi +done🧰 Tools
🪛 Shellcheck
[warning] 9-9: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
11-19: The container validation could use a tiny bit more robustnessWhile the container name validation is quite good, it might be worth adding timeout handling and docker daemon checks:
+# Check if docker daemon is running +if ! docker info >/dev/null 2>&1; then + echo "Error: Docker daemon is not running" >&2 + exit 1 +fi + # Get exact container name and verify it's running -container_name="$(docker ps --format '{{.Names}}' | grep '^care-db$')" +container_name="$(timeout 10s docker ps --format '{{.Names}}' | grep '^care-db$')"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
docs/databases/backup.rst(1 hunks)scripts/backup.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/databases/backup.rst
🧰 Additional context used
🪛 Shellcheck
scripts/backup.sh
[warning] 9-9: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
🔇 Additional comments (1)
scripts/backup.sh (1)
1-1: Verify script permissions
Let's ensure the script has proper execute permissions.
Fix cron job path and add verification
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
scripts/backup.sh (1)
1-9: I see we're living dangerously without error traps...While the initialization is decent, we might want to add some safety nets:
#!/bin/bash set -ueo pipefail + +# Cleanup function +cleanup() { + # Add cleanup tasks here + exit "${1:-0}" +} + +# Error handler +error() { + echo "Error: ${1:-'Unknown error'}" >&2 + cleanup 1 +} + +# Set up trap +trap 'error "Script interrupted"' INT TERM + # Ensure we can find the .env file -ENV_FILE="$(dirname "$(readlink -f "$0")")/../.env" +ENV_FILE="$(cd "$(dirname "$0")/.." && pwd)/.env"🧰 Tools
🪛 Shellcheck
[warning] 9-9: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
scripts/backup.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
scripts/backup.sh
[warning] 9-9: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
done |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
scripts/backup.sh (2)
1-9: I see we're trying to be careful with error handling... almost.While the setup is generally good, we could make it even better with a few tiny improvements:
#!/bin/bash set -ueo pipefail + +# Helper function for consistent error messages +error() { + echo "Error: $1" >&2 + exit 1 +} + # Ensure we can find the .env file ENV_FILE="$(dirname "$(readlink -f "$0")")/../.env" if [[ ! -f "${ENV_FILE}" ]]; then - echo "Error: .env file not found at ${ENV_FILE}" >&2 - exit 1 + error ".env file not found at ${ENV_FILE}" fi -source "${ENV_FILE}" +source "${ENV_FILE}" || error "Failed to source ${ENV_FILE}"🧰 Tools
🪛 Shellcheck
[warning] 9-9: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
1-42: Some architectural thoughts, if you're interested...A few suggestions to make this script more production-ready:
- The container name 'care-db' is hardcoded. Perhaps we could make it configurable?
- Consider adding proper logging instead of just echo statements
- Implement trap handlers for cleanup on script failure
- Add a dry-run mode for testing
Would you like me to provide implementation details for any of these suggestions?
🧰 Tools
🪛 Shellcheck
[warning] 9-9: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
scripts/backup.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
scripts/backup.sh
[warning] 9-9: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
🔇 Additional comments (2)
scripts/backup.sh (2)
10-17: 🛠️ Refactor suggestion
Oh, validation... how quaint.
While checking for empty variables is a start, perhaps we could be a tad more thorough:
# Validate required environment variables
required_vars=("POSTGRES_USER" "POSTGRES_DB" "DB_BACKUP_RETENTION_PERIOD")
for var in "${required_vars[@]}"; do
if [[ -z "${!var:-}" ]]; then
- echo "Error: $var environment variable is not set" >&2
- exit 1
+ error "$var environment variable is not set"
fi
done
+
+# Validate retention period
+if ! [[ "${DB_BACKUP_RETENTION_PERIOD}" =~ ^[0-9]+$ ]]; then
+ error "DB_BACKUP_RETENTION_PERIOD must be a positive integer"
+fi
+
+if [[ "${DB_BACKUP_RETENTION_PERIOD}" -lt 1 ]]; then
+ error "DB_BACKUP_RETENTION_PERIOD must be at least 1"
+fiLikely invalid or redundant comment.
34-42:
A backup without verification is just wishful thinking, isn't it?
Let's make this backup process a bit more... reliable:
# Remove old backup/backups
-docker exec -t ${container_name} find "/backups" -name "${POSTGRES_DB}_backup_*.dump" -type f -mtime +${DB_BACKUP_RETENTION_PERIOD} -exec rm {} \;
+if ! docker exec -t "${container_name}" find "/backups" -name "${POSTGRES_DB}_backup_*.dump" -type f -mtime +"${DB_BACKUP_RETENTION_PERIOD}" -exec rm {} \;; then
+ error "Failed to clean up old backups"
+fi
#backup the database
-if ! docker exec -t ${container_name} pg_dump -U ${POSTGRES_USER} -Fc -f /backups/${backup_file} ${POSTGRES_DB}; then
+if ! docker exec -t "${container_name}" pg_dump -U "${POSTGRES_USER}" -Fc -Z 9 -f "/backups/${backup_file}" "${POSTGRES_DB}"; then
echo "Error: Database backup failed" >&2
exit 1
fi
-echo "Backup of database '${POSTGRES_DB}' completed and saved as /backups/${backup_file}"
+
+# Verify backup size
+backup_size=$(docker exec -t "${container_name}" stat -f %z "/backups/${backup_file}")
+if [[ ${backup_size} -lt 1024 ]]; then
+ error "Backup file is suspiciously small (${backup_size} bytes)"
+fi
+
+# Verify backup integrity
+if ! docker exec -t "${container_name}" pg_restore -l "/backups/${backup_file}" >/dev/null 2>&1; then
+ error "Backup file appears to be corrupted"
+fi
+
+echo "Backup of database '${POSTGRES_DB}' completed and verified: /backups/${backup_file} (${backup_size} bytes)"Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
scripts/backup.sh (2)
1-9: Perhaps we could make the .env sourcing a bit more... robust?While the current implementation is decent, we could silence ShellCheck and add some validation:
#!/bin/bash set -ueo pipefail + +# shellcheck source=/dev/null source "${ENV_FILE}" + +# Validate required environment variables +required_vars=("POSTGRES_USER" "POSTGRES_DB" "DB_BACKUP_RETENTION_PERIOD") +for var in "${required_vars[@]}"; do + if [[ -z "${!var:-}" ]]; then + echo "Error: $var environment variable is not set" >&2 + exit 1 + fi +done🧰 Tools
🪛 Shellcheck
[warning] 9-9: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
21-22: That comment is stating the... obvious.The comment
#name the filedoesn't add any value since the code is self-documenting. We could remove it or make it more meaningful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
scripts/backup.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck
scripts/backup.sh
[warning] 9-9: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
Proposed Changes
Associated Issue
Architecture changes
Only PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Release Notes
New Features
Documentation
README.mdto include backup and restore instructions.docs/databases/backup.rst.CONTRIBUTING.md.Chores
docker-compose.yamlwith new volume mapping for backups.