chore: Builds + DB backups #180
Conversation
|
Warning Rate limit exceeded@gocanto has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 1 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. 📒 Files selected for processing (4)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a database backup subsystem (scripts, Makefile targets, docs), cron automation, externalizes the DB Docker volume to an existing named volume, and moves Caddy metrics to global scope while removing forwarding of X-Forwarded-* headers in prod reverse_proxy blocks. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as make (backup:create)
participant Script as infra/scripts/db-backup.sh
participant Docker as Docker Engine
participant DB as PostgreSQL
participant FS as Filesystem
User->>CLI: make backup:create
CLI->>Script: execute
Script->>Docker: ensure container running
Docker-->>Script: status OK
Script->>Docker: read DB creds (docker secrets)
Docker-->>Script: creds
Script->>Docker: run pg_dump inside container
Docker->>DB: pg_dump request
DB-->>Docker: SQL stream
Docker-->>Script: SQL output
Script->>FS: write timestamped file (optional gzip)
FS-->>Script: file created
Script-->>User: success + filename
sequenceDiagram
actor User
participant CLI as make (backup:restore)
participant Script as infra/scripts/db-backup.sh
participant FS as Filesystem
participant Docker as Docker Engine
participant DB as PostgreSQL
User->>CLI: make backup:restore FILE=<file>
CLI->>Script: execute
Script->>FS: validate file (decompress if gz)
FS-->>Script: SQL ready
Script->>User: confirm destructive restore
User-->>Script: confirm
Script->>Docker: read DB creds
Docker-->>Script: creds
Script->>Docker: stream SQL into psql
Docker->>DB: restore
DB-->>Docker: done
Script-->>User: restore finished
sequenceDiagram
actor User
participant CLI as make (backup:cron:setup)
participant CronScript as infra/scripts/setup-cron-backup.sh
participant FS as Filesystem
participant System as crontab
User->>CLI: make backup:cron:setup
CLI->>CronScript: execute
CronScript->>CronScript: verify db-backup.sh present & executable
CronScript->>FS: create log dir
FS-->>CronScript: ready
CronScript->>CronScript: generate cron entries (backup + cleanup)
alt dry-run
CronScript-->>User: preview entries
else install
CronScript->>System: read current crontab
System-->>CronScript: current jobs
CronScript->>System: install updated crontab
System-->>CronScript: success
CronScript-->>User: installed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Summary of ChangesHello @gocanto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on refining infrastructure configurations. It updates the Docker Compose setup to utilize an external volume for the database, streamlining volume management. Additionally, it refactors Caddy server configurations by adjusting how metrics are enabled and simplifying header forwarding for proxied services, aiming for cleaner and potentially more efficient proxy behavior. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several beneficial updates to the infrastructure configuration. The change to an external Docker volume for the database is a good practice, though I've suggested documenting the new manual setup step this requires. The Caddy configurations have been modernized and simplified, which improves maintainability and aligns them with current best practices. Overall, these are solid improvements.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infra/caddy/Caddyfile.prod (1)
75-95: Explicitly set X-Forwarded headers in reverse proxy blocks.The backend code explicitly depends on these headers:
ParseClientIP()prefersX-Forwarded-Forfor client IP extraction (pkg/portal/support.go:174)GenerateURL()usesX-Forwarded-Protofor scheme determination andX-Forwarded-Hostfor host reconstruction (pkg/portal/support.go:108-115)While Caddy's
reverse_proxydirective sets these headers by default, relying on implicit defaults is unreliable for production. Add explicitheader_updirectives in both reverse proxy blocks:
header_up X-Forwarded-For {http.request.header.X-Forwarded-For}header_up X-Forwarded-Proto {http.request.header.X-Forwarded-Proto}header_up X-Forwarded-Host {http.request.header.X-Forwarded-Host}Without these headers: client IP detection fails, URL generation may produce incorrect schemes, and access logging loses the original client context.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
MakefileREADME.mddocker-compose.ymldocs/DB_BACKUPS.mddocs/SETUP.mdinfra/caddy/Caddyfile.localinfra/caddy/Caddyfile.prodinfra/makefile/backup.mkinfra/scripts/db-backup.shinfra/scripts/setup-cron-backup.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T09:16:09.386Z
Learnt from: gocanto
Repo: oullin/api PR: 163
File: infra/caddy/Caddyfile.local:7-7
Timestamp: 2025-11-17T09:16:09.386Z
Learning: In Caddy v2, the `metrics` directive should be placed directly in the global options block (e.g., `{ metrics }`), not nested inside a `servers` block. The nested `servers { metrics }` syntax is deprecated.
Applied to files:
infra/caddy/Caddyfile.prodinfra/caddy/Caddyfile.local
🪛 LanguageTool
docs/DB_BACKUPS.md
[typographical] ~82-~82: To join two clauses or introduce examples, consider using an em dash.
Context: ...--| | -h, --help | Show help message | - | | -f, --file FILE | Backup file to r...
(DASH_RULE)
[typographical] ~83-~83: To join two clauses or introduce examples, consider using an em dash.
Context: ...file to restore (required for restore) | - | | -c, --compress BOOL | Compress bac...
(DASH_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/DB_BACKUPS.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
32-32: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
39-39: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
42-42: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (1.25.5)
🔇 Additional comments (16)
docker-compose.yml (1)
24-28: LGTM! External volume configuration is well-documented.The switch to an external volume (
api_oullin_db_data) is appropriate for production data management. The prerequisite documentation has been added in bothdocs/SETUP.mdandREADME.mdQuick Start, addressing the onboarding concern from the previous review.infra/scripts/db-backup.sh (1)
1-42: Good use of strict mode and structured logging.The script follows shell best practices with
set -euo pipefailfor strict error handling, organized configuration, and consistent colored logging functions.Makefile (2)
45-45: LGTM! Clean integration of backup subsystem.The backup makefile is properly included, extending the build system with backup capabilities.
88-96: Help text is well-formatted and consistent.The backup commands help section follows the established format and provides clear descriptions.
docs/SETUP.md (1)
8-8: LGTM! Prerequisites clearly documented.The one-time Docker volume creation step is appropriately documented alongside other prerequisites.
README.md (2)
24-26: LGTM! Quick Start flow is clear and logical.The updated steps properly sequence the one-time volume creation before running the application.
36-36: Good addition of backup documentation link.The new documentation entry for database backups is appropriately placed in the "Get Started" section.
infra/makefile/backup.mk (2)
1-9: LGTM! Configuration and target declarations are well-structured.The
.PHONYdeclarations and configuration variables follow good Makefile conventions. Using?=forBACKUP_RETENTION_DAYSallows environment override while providing a sensible default.
67-99: Comprehensive help documentation.The help target provides clear usage examples with proper formatting, consistent with the main Makefile's help style.
infra/scripts/setup-cron-backup.sh (5)
1-23: LGTM!Good use of
set -euo pipefailfor strict error handling. The path resolution and color constant definitions are solid.
24-35: LGTM!Logging functions are clean and correctly direct error output to stderr.
36-85: LGTM!The usage documentation is comprehensive with helpful examples, and prerequisite checks cover the essential validations.
158-166: LGTM!The function works as intended. The
-A 5context should be sufficient for the generated entries.
242-266: LGTM!The action execution logic is clean, with helpful completion messages for the user.
infra/caddy/Caddyfile.prod (1)
9-11: LGTM! Metrics directive correctly positioned.The metrics directive is now properly placed in the global options block, which aligns with Caddy v2 best practices.
Based on learnings, this is the recommended approach over the deprecated nested
servers { metrics }syntax.infra/caddy/Caddyfile.local (1)
9-11: LGTM! Metrics directive correctly positioned.The metrics directive is properly placed in the global options block, consistent with the production configuration and Caddy v2 best practices.
Based on learnings, this approach is preferred over the deprecated nested
servers { metrics }syntax.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/DB_BACKUPS.mdinfra/makefile/backup.mkinfra/scripts/db-backup.shinfra/scripts/setup-cron-backup.sh
🧰 Additional context used
🧬 Code graph analysis (1)
infra/scripts/db-backup.sh (1)
infra/scripts/setup-cron-backup.sh (5)
log_error(32-34)log_info(24-26)log_warn(28-30)show_usage(44-75)main(203-274)
🪛 LanguageTool
docs/DB_BACKUPS.md
[grammar] ~4-~4: Ensure spelling is correct
Context: ...kup and restore utility that works with Dockerised databases. ### Features - Backup:...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[typographical] ~113-~113: To join two clauses or introduce examples, consider using an em dash.
Context: ...--| | -h, --help | Show help message | - | | -f, --file FILE | Backup file to r...
(DASH_RULE)
[typographical] ~114-~114: To join two clauses or introduce examples, consider using an em dash.
Context: ...file to restore (required for restore) | - | | -c, --compress BOOL | Compress bac...
(DASH_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/DB_BACKUPS.md
1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
6-6: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
68-68: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
82-82: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
91-91: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
98-98: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
184-184: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
184-184: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
213-213: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
214-214: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
214-214: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
216-216: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
218-218: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
222-222: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
223-223: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
223-223: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
225-225: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
227-227: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
231-231: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
232-232: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
232-232: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
234-234: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
239-239: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
240-240: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
251-251: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
252-252: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
269-269: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
270-270: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (13)
infra/scripts/db-backup.sh (4)
1-28: LGTM on configuration section.The
readonlyissue from past reviews has been addressed -BACKUP_DIRandRETENTION_DAYSare now mutable to allow CLI flag overrides, whileCONTAINER_NAMEremainsreadonlyas it's not CLI-configurable.
143-152: LGTM ---cleanflag properly included.The
pg_dumpcommand now includes--clean(line 149), which addresses the previous review concern about restore failures on existing tables. This ensuresDROPstatements are included in the backup.
181-233: LGTM on restore_backup function.The function properly handles path resolution, file existence checks, user confirmation, and both compressed/uncompressed formats. The
--cleanflag increate_backupensures restores work correctly on existing databases.
295-320: LGTM on cleanup_old_backups.The function properly validates
RETENTION_DAYSas numeric, usesfindwith-print0for safe filename handling, and provides clear logging of removed files.infra/scripts/setup-cron-backup.sh (5)
36-42: LGTM -require_arghelper properly implemented.The helper function addresses the past review comment about validating arguments before accessing them. It's now used consistently for
-s,-r, and--cleanup-scheduleoptions.
108-119: LGTM - Cron entry paths are now quoted.The paths in the generated cron entries (lines 113, 116) are now properly quoted to handle spaces, addressing the previous review feedback.
140-157: LGTM - Temp file cleanup with trap.The
trap 'rm -f "${temp_cron}"' RETURN(line 143) ensures the temp file is cleaned up when the function returns, addressing the previous review concern about leaked temp files.
186-201: LGTM on remove_cron_jobs.The function now uses a
RETURNtrap for temp file cleanup (line 191), consistent withinstall_cron_jobs.
217-231: LGTM - Argument validation in place.All options requiring values (
-s,-r,--cleanup-schedule) now callrequire_argto validate the argument exists before accessing it, addressing the previous review feedback.infra/makefile/backup.mk (3)
19-27: LGTM - File path is now quoted.The
$(file)parameter is now properly quoted on line 27 (--file "$(file)"), addressing the previous review comment about handling paths with spaces.
40-54: LGTM on cron setup targets.The
$(schedule)parameter is properly quoted to handle cron expressions containing spaces. The$(BACKUP_RETENTION_DAYS)is expected to be numeric and is validated by the underlying script.
1-9: Parent Makefile properly defines all required variables.The parent Makefile correctly defines
ROOT_PATH,STORAGE_PATH, and all color variables (GREEN,RED,YELLOW,NC,BOLD,CYAN,BLUE) that this file references. The file is also properly included viainclude ./infra/makefile/backup.mkin the main Makefile.docs/DB_BACKUPS.md (1)
259-267: LGTM - Username placeholder properly used.The example now uses
<your_username>placeholder (line 260) and provides instructions for retrieving the actual username from Docker secrets (lines 263-267), addressing the previous review feedback.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive database backup and restore system, including shell scripts, Makefile integration, and documentation. The changes are well-structured and the documentation is thorough. My review focuses on a few key areas:
- Security: I've identified a critical IP spoofing vulnerability in the Caddy configurations that should be addressed.
- Robustness: The new shell scripts have some areas for improvement in argument parsing and structure to make them more robust and maintainable.
Overall, this is a great addition to the project's operational capabilities. Addressing the feedback will enhance the security and quality of the new tooling.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
infra/caddy/Caddyfile.prod (1)
108-111: Same X-Forwarded header concern applies to the web proxy.The default web handler has the same X-Forwarded header passthrough pattern that may not work correctly if Caddy is the edge proxy.
Please apply the same verification and fix from the API proxy block (lines 76-82) to this web proxy configuration to ensure consistent behavior.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
infra/caddy/Caddyfile.localinfra/caddy/Caddyfile.prodinfra/scripts/db-backup.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-17T09:16:09.386Z
Learnt from: gocanto
Repo: oullin/api PR: 163
File: infra/caddy/Caddyfile.local:7-7
Timestamp: 2025-11-17T09:16:09.386Z
Learning: In Caddy v2, the `metrics` directive should be placed directly in the global options block (e.g., `{ metrics }`), not nested inside a `servers` block. The nested `servers { metrics }` syntax is deprecated.
Applied to files:
infra/caddy/Caddyfile.prodinfra/caddy/Caddyfile.local
🧬 Code graph analysis (1)
infra/scripts/db-backup.sh (1)
infra/scripts/setup-cron-backup.sh (5)
log_error(32-34)log_info(24-26)log_warn(28-30)show_usage(44-75)main(203-274)
🔇 Additional comments (14)
infra/scripts/db-backup.sh (12)
16-18: Excellent fix—CLI override now works.The removal of
readonlyfromBACKUP_DIRandRETENTION_DAYSresolves the critical issue flagged in previous reviews. Lines 360 and 368 can now successfully reassign these variables when--retentionor--dirflags are provided.
43-57: LGTM! Cross-platform stat handling is robust.The
stat_epochandstat_datefunctions correctly handle both GNU (stat -c) and BSD (stat -f) variants, ensuring the script works on Linux and macOS.
102-108: LGTM! Container check is clear and helpful.The function correctly verifies the container is running and provides actionable guidance on how to start it if it's not.
110-121: LGTM! Secure credential handling.Reading credentials from Docker secrets (
/run/secrets/pg_usernameand/run/secrets/pg_dbname) follows security best practices. The validation ensures both credentials are present before proceeding.
123-179: Excellent implementation—--cleanflag added per previous review.The addition of
--cleanat line 149 ensurespg_dumpincludes DROP statements, resolving the past concern about restore failures on existing tables. Theumask 077at line 132 properly restricts backup file permissions for security.
181-233: LGTM! Restore logic is solid.The function correctly handles both compressed and uncompressed backups, validates input, and prompts for confirmation before destructive operations. The relative path resolution (lines 191-193) is particularly helpful for user convenience.
244-249: Excellent fix—tab character syntax corrected.Line 248 now correctly uses
"${epoch}"$'\t'"${file}"with an actual tab character ($'\t'), resolving the issue flagged in previous reviews where"\t"created a literal backslash-t string instead of a tab.
259-287: LGTM! Dual-path sort handles platform differences.The implementation gracefully handles systems with and without
sort -zsupport. The null-delimiter path (lines 259-272) safely processes filenames with special characters, while the fallback (lines 273-287) works on older systems.
295-320: LGTM! Cleanup logic is correct and safe.The retention validation (line 301) and the
find -mtime "+${RETENTION_DAYS}"syntax (line 313) correctly identify and remove backups older than the retention period. Logging each removal provides a useful audit trail.
335-370: Excellent addition—comprehensive argument validation.The validation for
-f|--file,-c|--compress,-r|--retention, and-d|--dirflags resolves the concerns raised in previous reviews. The pattern[[ $# -lt 2 || "$2" == -* ]]safely handles missing arguments and prevents treating flags as values, all while respectingset -u.
383-410: LGTM! Command dispatch is clean and correct.The validation ensures a command is specified (lines 384-388), and the case statement properly routes to the corresponding function with appropriate arguments.
1-412: Outstanding work—all previous concerns addressed!This script demonstrates excellent craftsmanship:
✅ Past critical issue resolved: Removed
readonlyfromBACKUP_DIRandRETENTION_DAYS(lines 16, 18)
✅ Past major issue resolved: Added--cleanflag topg_dump(line 149)
✅ Past issue resolved: Fixed tab character syntax inlist_backups(line 248)
✅ Past issue resolved: Added comprehensive argument validation (lines 336-370)Strengths:
- Proper error handling with
set -euo pipefail- Security-conscious (umask 077, Docker secrets)
- Cross-platform stat compatibility (lines 43-57)
- Handles both compressed and uncompressed backups
- Clear logging and user feedback
The suggested improvements are optional refinements and do not block this implementation.
infra/caddy/Caddyfile.local (1)
9-11: LGTM! Metrics directive correctly moved to global scope.This change aligns with Caddy v2 best practices and resolves the deprecated nested
servers { metrics }syntax.Based on learnings, the metrics directive should be placed directly in the global options block, which is now correctly implemented.
infra/caddy/Caddyfile.prod (1)
9-11: LGTM! Metrics directive correctly moved to global scope.This change aligns with Caddy v2 best practices and resolves the deprecated nested
servers { metrics }syntax, consistent with the local configuration.Based on learnings, the metrics directive should be placed directly in the global options block, which is now correctly implemented.
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive database backup and restore solution, including scripts, Makefile commands, and excellent documentation. The change to an external Docker volume for the database is a good step for data safety.
My review has identified a critical security vulnerability in the updated Caddy configurations, which allows for IP spoofing by trusting client-provided headers. I've also noted some areas for improvement in the argument parsing of the db-backup.sh script to make it more robust and user-friendly. Please address the critical security issues before merging.
|
/gemini review |
|
@codex review |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive database backup and restore solution, including scripts for manual operations, Makefile targets for convenience, and a script to set up automated cron jobs. The documentation is thorough, and the changes to Docker Compose to use an external volume for the database are a good step towards more robust data management. The shell scripts are well-written, with good error handling and portability considerations. I've provided a few suggestions to further improve the new scripts by removing some redundant code and optimizing a file counting operation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary by CodeRabbit
New Features
Documentation
Infrastructure & Operations
Chores
✏️ Tip: You can customize this high-level summary in your review settings.