infra(cpb): rewrite DB setup and add schema for Connecting People Bot#122
Closed
SashkoMarchuk wants to merge 3 commits intomainfrom
Closed
infra(cpb): rewrite DB setup and add schema for Connecting People Bot#122SashkoMarchuk wants to merge 3 commits intomainfrom
SashkoMarchuk wants to merge 3 commits intomainfrom
Conversation
…ple Bot Replace the old setup script that required postgres admin access and CREATEROLE with a version that uses temporal user (CREATEDB) and grants to existing n8n user. Add complete 6-table schema (cycles, opt_in_responses, pairings, pair_history, interactions, admin_reports) with idempotent IF NOT EXISTS, triggers, and indexes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🤖 CodeRabbit AI Review AvailableTo request a code review from CodeRabbit AI, add CodeRabbit will analyze your code and provide feedback on:
Note: Reviews are only performed when |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces the removed top-level script Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as Installer (scripts/cpb/setup-db.sh)
participant PG as PostgreSQL Server
participant Temporal as Temporal/admin user
participant AppUser as n8n user
rect rgba(135,206,235,0.5)
Installer->>PG: Connect using Temporal credentials (host, port)
Note right of PG: Authentication/connection
end
rect rgba(144,238,144,0.5)
Installer->>PG: IF database NOT EXISTS -> CREATE DATABASE OWNER Temporal
PG-->>Installer: OK / error
end
rect rgba(255,228,196,0.5)
Installer->>PG: GRANT ALL PRIVILEGES ON DATABASE TO AppUser
PG-->>Installer: OK / error
end
rect rgba(221,160,221,0.5)
Installer->>PG: Attempt GRANT ALL ON SCHEMA public TO AppUser
PG-->>Installer: OK / permission denied (ignored) / unexpected error (fail)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
scripts/cpb/setup-db.sh (1)
88-101: Consider using explicit if-else for robustness.The
A && B || Cpattern on lines 88-91 can behave unexpectedly: ifpsqlsucceeds butechofails (rare but possible), the error block executes. Per the static analysis hint (SC2015), this is not a true if-then-else.A more robust pattern:
-SCHEMA_GRANT_ERR=$(PGPASSWORD="${TEMPORAL_PASS}" psql -v ON_ERROR_STOP=1 \ - -h "$PGHOST" -p "$PGPORT" -U "$TEMPORAL_USER" -d "$CPB_DB" \ - -c "GRANT ALL ON SCHEMA public TO \"${N8N_USER}\";" 2>&1) && \ - echo " Schema grant on public: OK" || { - if echo "$SCHEMA_GRANT_ERR" | grep -qi "permission denied\|must be owner"; then +if SCHEMA_GRANT_ERR=$(PGPASSWORD="${TEMPORAL_PASS}" psql -v ON_ERROR_STOP=1 \ + -h "$PGHOST" -p "$PGPORT" -U "$TEMPORAL_USER" -d "$CPB_DB" \ + -c "GRANT ALL ON SCHEMA public TO \"${N8N_USER}\";" 2>&1); then + echo " Schema grant on public: OK" +elif echo "$SCHEMA_GRANT_ERR" | grep -qi "permission denied\|must be owner"; then echo " Schema grant on public: skipped (not needed on PG14 — PUBLIC has CREATE by default)" echo " NOTE: After upgrading to PG15+, re-run this script or grant manually:" echo " GRANT ALL ON SCHEMA public TO \"${N8N_USER}\";" - else +else echo "ERROR: Schema grant failed unexpectedly:" >&2 echo " $SCHEMA_GRANT_ERR" >&2 exit 1 - fi -} +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cpb/setup-db.sh` around lines 88 - 101, Replace the fragile "A && B || C" flow around SCHEMA_GRANT_ERR with an explicit if/then/else: run the psql command (the invocation that assigns SCHEMA_GRANT_ERR using PGPASSWORD, -h "$PGHOST" -p "$PGPORT" -U "$TEMPORAL_USER" -d "$CPB_DB" -c "GRANT ALL ON SCHEMA public TO \"${N8N_USER}\";") and test its exit status with if …; then echo the success message ("Schema grant on public: OK"); else inspect SCHEMA_GRANT_ERR for the permission/owner strings and run the existing skip message or print the error and exit 1—this removes the brittle dependency on echo succeeding and follows the SCHEMA_GRANT_ERR/psql invocation and N8N_USER variable references in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/cpb/setup-db.sh`:
- Around line 24-26: The usage example's final command reference is incorrect;
update the usage line that shows "CPB_POSTGRES_HOST=...
POSTGRES_PASSWORD_TEMPORAL=... POSTGRES_USER_N8N=... ./scripts/cpb-setup-db.sh"
to point to the correct script location by replacing "./scripts/cpb-setup-db.sh"
with "./scripts/cpb/setup-db.sh" so the example matches the actual script name
and path.
In `@sql/cpb/init-schema.sql`:
- Around line 1-16: Update the Usage comment to reference the correct SQL
filename: replace "cpb-init-schema.sql" with "init-schema.sql" in the header
block so the Usage line accurately reflects the actual file name; locate the
Usage text in the file header comment near the top (the comment containing
"Usage: psql -h <host> -U cpb_app -d cpb_bot -f ...") and make the
filename correction.
---
Nitpick comments:
In `@scripts/cpb/setup-db.sh`:
- Around line 88-101: Replace the fragile "A && B || C" flow around
SCHEMA_GRANT_ERR with an explicit if/then/else: run the psql command (the
invocation that assigns SCHEMA_GRANT_ERR using PGPASSWORD, -h "$PGHOST" -p
"$PGPORT" -U "$TEMPORAL_USER" -d "$CPB_DB" -c "GRANT ALL ON SCHEMA public TO
\"${N8N_USER}\";") and test its exit status with if …; then echo the success
message ("Schema grant on public: OK"); else inspect SCHEMA_GRANT_ERR for the
permission/owner strings and run the existing skip message or print the error
and exit 1—this removes the brittle dependency on echo succeeding and follows
the SCHEMA_GRANT_ERR/psql invocation and N8N_USER variable references in the
diff.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d0996595-ad2d-40dc-8c9d-8b17e5f32a7b
📒 Files selected for processing (3)
scripts/cpb-setup-db.shscripts/cpb/setup-db.shsql/cpb/init-schema.sql
💤 Files with no reviewable changes (1)
- scripts/cpb-setup-db.sh
Fix two filename references in header comments flagged by CodeRabbit: - setup-db.sh usage example: cpb-setup-db.sh → cpb/setup-db.sh - init-schema.sql usage example: cpb-init-schema.sql → init-schema.sql Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/cpb/setup-db.sh (1)
88-101: Consider restructuringA && B || Cpattern for robustness.Shellcheck correctly notes that if the
echo " Schema grant on public: OK"command were to fail (unlikely but possible if stdout is closed), the error-handling block would execute despite thepsqlcommand succeeding. Since the error message wouldn't contain "permission denied", the script would incorrectlyexit 1.♻️ Suggested refactor using explicit if-then-else
-SCHEMA_GRANT_ERR=$(PGPASSWORD="${TEMPORAL_PASS}" psql -v ON_ERROR_STOP=1 \ +if SCHEMA_GRANT_ERR=$(PGPASSWORD="${TEMPORAL_PASS}" psql -v ON_ERROR_STOP=1 \ -h "$PGHOST" -p "$PGPORT" -U "$TEMPORAL_USER" -d "$CPB_DB" \ - -c "GRANT ALL ON SCHEMA public TO \"${N8N_USER}\";" 2>&1) && \ - echo " Schema grant on public: OK" || { + -c "GRANT ALL ON SCHEMA public TO \"${N8N_USER}\";" 2>&1); then + echo " Schema grant on public: OK" +else if echo "$SCHEMA_GRANT_ERR" | grep -qi "permission denied\|must be owner"; then echo " Schema grant on public: skipped (not needed on PG14 — PUBLIC has CREATE by default)" echo " NOTE: After upgrading to PG15+, re-run this script or grant manually:" echo " GRANT ALL ON SCHEMA public TO \"${N8N_USER}\";" else echo "ERROR: Schema grant failed unexpectedly:" >&2 echo " $SCHEMA_GRANT_ERR" >&2 exit 1 fi -} +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cpb/setup-db.sh` around lines 88 - 101, Replace the fragile "A && B || C" construct around SCHEMA_GRANT_ERR/psql with an explicit if/then/else: run the psql command (the same invocation that assigns SCHEMA_GRANT_ERR) and check its exit status with if psql ...; then echo the success message (" Schema grant on public: OK"); else capture the stderr output into SCHEMA_GRANT_ERR and run the existing permission-check (grep -qi "permission denied\|must be owner") to either print the skipped/upgrade note or print the error and exit 1; update references to SCHEMA_GRANT_ERR and the exact GRANT command string to match the current block so logic and messages remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/cpb/setup-db.sh`:
- Around line 88-101: Replace the fragile "A && B || C" construct around
SCHEMA_GRANT_ERR/psql with an explicit if/then/else: run the psql command (the
same invocation that assigns SCHEMA_GRANT_ERR) and check its exit status with if
psql ...; then echo the success message (" Schema grant on public: OK"); else
capture the stderr output into SCHEMA_GRANT_ERR and run the existing
permission-check (grep -qi "permission denied\|must be owner") to either print
the skipped/upgrade note or print the error and exit 1; update references to
SCHEMA_GRANT_ERR and the exact GRANT command string to match the current block
so logic and messages remain identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 849e7db5-6419-42eb-9b1f-455b108bb0e1
📒 Files selected for processing (2)
scripts/cpb/setup-db.shsql/cpb/init-schema.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- sql/cpb/init-schema.sql
Refactor schema grant error handling from fragile `cmd && echo || { ... }`
to proper if/then/else. Prevents false error-path execution if echo fails.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Collaborator
Author
|
Superseded by new PR that uses master password to create dedicated CPB role/user |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
setup-db.sh: Old script assumedpostgresadmin access andCREATEROLE— both unavailable on our RDS. New version usestemporaluser (hasCREATEDB) and grants to existingn8nuser. Moved fromscripts/cpb-setup-db.sh→scripts/cpb/setup-db.sh.sql/cpb/init-schema.sql: Complete 6-table schema for CPB Slack bot —cycles,opt_in_responses,pairings,pair_history,interactions,admin_reports.set -eo pipefail,ON_ERROR_STOP=1,:?fail-fast on required env vars.IF NOT EXISTS,DROP TRIGGER IF EXISTS,CREATE OR REPLACE. Safe to re-run — double-run produces 0 errors.Test plan
/ultra --mediumadversarial code review: 0 CRITICAL/HIGH issues, production-ready verdict (9-10/10 across all confidence dimensions)Summary by CodeRabbit
Chores
New Features