Skip to content

feat!: add dependencies to role creation process#17

Open
westonplatter wants to merge 18 commits intomainfrom
feat/app-setup-read-write
Open

feat!: add dependencies to role creation process#17
westonplatter wants to merge 18 commits intomainfrom
feat/app-setup-read-write

Conversation

@westonplatter
Copy link
Copy Markdown
Member

@westonplatter westonplatter commented Jan 29, 2026

Breaking Change

This is a breaking change given it changes the roles variable and updates the following nested items from an object to a list of objects,

  • schema_grants
  • sequence_grants
  • table_grants

what

  • Base roles (base_roles_map):

    • Roles with no roles attribute, OR
    • Roles that only inherit from built-in PostgreSQL roles (like pg_monitor, pg_read_all_data)
  • Dependent roles (dependent_roles_map):

    • Roles that inherit from at least one custom role defined in this module
    • May also inherit from built-in roles (mixed is fine)
    • Use bullet points to be concise and to the point.

why

Added the base + dependent role hierarchy to resolve issues when building out the ./examples/llm_chat_app example

Summary by CodeRabbit

  • New Features

    • Split role handling into distinct base and dependent roles and added three outputs exposing role sets.
  • Examples

    • Added a complete LLM chat app example with Terraform configuration, variables, provider setup, and end-to-end automation scripts for apply, tests, and cleanup.
  • Documentation

    • New README and test instructions with architecture diagrams, setup, verification steps, and cleanup guidance.
  • Tests

    • Added test suite validating role classification and related behaviors.

✏️ Tip: You can customize this high-level summary in your review settings.

@westonplatter westonplatter requested a review from a team as a code owner January 29, 2026 23:42
@westonplatter westonplatter requested a review from dudymas January 29, 2026 23:42
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 29, 2026

📝 Walkthrough

Walkthrough

This PR refactors role handling by replacing a single postgresql_role.role with two resources: postgresql_role.base_role (roles without custom dependencies) and postgresql_role.dependent_role (roles that inherit from other custom roles). Locals were added to classify built-in vs. custom roles and to build maps for base and dependent roles; dependences and grants were updated to reference both role sets. Three new outputs (roles, base_roles, dependent_roles) were added. A complete example examples/llm_chat_app was introduced with Terraform configs, fixtures, provider/version files, scripts, tests, and documentation demonstrating RBAC across multiple schemas.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title 'feat!: add dependencies to role creation process' directly and clearly summarizes the main change—introducing role hierarchy with base and dependent role splits to resolve dependency issues during role provisioning.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/app-setup-read-write

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
main.tf (1)

170-181: Inconsistent depends_on for table_access resource.

All other grant resources (database_access, schema_access, sequence_access, default_privileges) include both postgresql_role.base_role and postgresql_role.dependent_role in their depends_on. However, table_access only depends on postgresql_database.logical_dbs.

If table grants can reference roles created by this module, this inconsistency could cause race conditions during apply.

🔧 Proposed fix
 resource "postgresql_grant" "table_access" {
   for_each = local.table_grants_map

   role        = each.value.role
   database    = each.value.database
   schema      = each.value.schema
   object_type = each.value.object_type
   privileges  = each.value.privileges
   objects     = each.value.objects

-  depends_on = [postgresql_database.logical_dbs]
+  depends_on = [postgresql_database.logical_dbs, postgresql_role.base_role, postgresql_role.dependent_role]
 }
🤖 Fix all issues with AI agents
In `@examples/llm_chat_app/1_apply_terraform.sh`:
- Line 11: The script contains a hardcoded cd to /Users/weston/... which breaks
portability; replace the literal path used by the cd command with a path
computed from the script location or a repository-relative path so the script
works for other devs and CI (e.g., derive the script directory using dirname $0
and then cd into the examples/llm_chat_app directory, or use a relative path
from the repo root).

In `@examples/llm_chat_app/RUN_ALL_TESTS.sh`:
- Line 6: Replace the hardcoded SCRIPT_DIR assignment in RUN_ALL_TESTS.sh with a
dynamic determination using the script's actual location: set SCRIPT_DIR (the
variable currently holding "/Users/weston/...") by resolving the script path
(e.g., using BASH_SOURCE or $0 with dirname and cd/pwd) so the script works
portably on other machines; update any references that rely on SCRIPT_DIR
accordingly.

In `@examples/llm_chat_app/run_tests.sh`:
- Around line 76-147: The PGUSER values used in tests 2–8 are wrong (they use
group roles like role_service_*); update each PGUSER assignment to the
corresponding login role from the fixtures: replace
PGUSER=role_service_migration → PGUSER=service_migrator,
PGUSER=role_service_fastapi_rw → PGUSER=service_fastapi_rw,
PGUSER=role_service_fastapi_ro → PGUSER=service_fastapi_ro,
PGUSER=role_service_pipeline_rw → PGUSER=service_pipeline_rw, and
PGUSER=role_service_pipeline_ro → PGUSER=service_pipeline_ro in the Test 2..8
blocks (look for the PGUSER=... lines inside the Test 2–8 psql -c "..."
sections).
- Around line 34-66: The script uses non-login roles as PGUSER (e.g.,
PGUSER=role_service_migration) which cannot authenticate; update every PGUSER
assignment in run_tests.sh to use the corresponding login roles from fixtures
(replace role_service_migration → service_migrator, role_service_fastapi_rw →
service_fastapi_rw, role_service_fastapi_ro → service_fastapi_ro,
role_service_pipeline_rw → service_pipeline_rw, role_service_pipeline_ro →
service_pipeline_ro), ensuring each psql here-doc and any other PGUSER
environment usages reference the login role names instead of the non-login group
role names.
- Line 11: Replace the hardcoded absolute cd path in run_tests.sh with logic
that derives the script's directory at runtime and cds there (i.e., compute the
script file's directory using the script's source path like dirname/BASH_SOURCE
and change into that directory) so the script is portable across machines;
update the line that performs cd /Users/... to use this computed script-dir
instead.

In `@examples/llm_chat_app/TEST_INSTRUCTIONS.md`:
- Around line 134-135: The comment notes PostgreSQL doesn't allow wildcards in
DROP ROLE; replace the single-line suggestion `DROP ROLE IF EXISTS
role_service_* CASCADE;` with instructions to programmatically drop matching
roles: run a query against pg_roles filtering rolname LIKE 'role_service_%' to
generate DROP ROLE ... CASCADE statements and then execute those statements (or
pipe them back into psql), or iterate in a script to call DROP ROLE for each
matching role; update the TEST_INSTRUCTIONS.md text accordingly and keep the
existing alternative `tofu destroy -auto-approve` note.
- Around line 139-141: Replace the hardcoded absolute path string
"/Users/weston/clients/masterpoint/terraform-postgres-config-dbs-users-roles/docs/llm_app_example.md"
in TEST_INSTRUCTIONS.md with a repository-relative path (for example
"docs/llm_app_example.md" or "./docs/llm_app_example.md") so the architecture
reference is portable; update the markdown link text if present to use the
relative path and verify the link resolves in the repo.
- Around line 19-23: The README uses a hardcoded absolute path to run the
example which will fail for other users; update TEST_INSTRUCTIONS.md to use a
relative invocation or instruct users to first change into the example directory
before running the script (reference the RUN_ALL_TESTS.sh call in the doc) —
e.g., replace the absolute cd path with a relative path like changing to the
examples/llm_chat_app folder or add a preceding instruction to cd into that
directory so the chmod and ./RUN_ALL_TESTS.sh commands work for all users.
- Around line 115-118: Replace the hardcoded absolute path in
TEST_INSTRUCTIONS.md (the line that starts with cd
/Users/weston/.../examples/llm_chat_app) with a relative path to the example
directory so the command works across environments; specifically change that cd
invocation to use a relative path to examples/llm_chat_app (and leave the
subsequent tofu destroy -auto-approve command unchanged).
🧹 Nitpick comments (6)
main.tf (1)

79-103: Consider extracting common role attributes to reduce duplication.

Both base_role and dependent_role resources share identical attribute mappings (lines 83-100 and 109-126). While functional, this creates maintenance overhead.

💡 Optional: Use a local for shared attribute mapping

You could define a helper local to reduce duplication, though this is a stylistic preference:

# In locals block
roles_config = {
  for name, data in merge(local.base_roles_map, local.dependent_roles_map) : name => {
    name                      = data.role.name
    superuser                 = data.role.superuser
    # ... other attributes
  }
}

Alternatively, keep as-is if explicit attribute mapping per resource type is preferred for clarity.

Also applies to: 105-129

examples/llm_chat_app/3_run_verification_tests.sh (1)

15-16: Test numbering starts at 2.

Tests are numbered 2-8, but there's no Test 1 in this script. Consider either adding Test 1 or renumbering the tests sequentially starting from 1 for consistency. This appears intentional (perhaps Test 1 is the Terraform apply itself), but a brief comment explaining the numbering would help future maintainers.

examples/llm_chat_app/outputs.tf (1)

3-21: Add descriptions to outputs for better documentation.

Following Terraform best practices and Cloud Posse conventions, outputs should include descriptions to help users understand what each output provides.

♻️ Example with descriptions
 output "databases" {
+  description = "Map of created PostgreSQL databases"
   value = module.postgres_automation.databases
 }

 output "database_access" {
+  description = "Database-level access grants"
   value = module.postgres_automation.database_access
 }

 output "default_privileges" {
+  description = "Default privileges configured for roles"
   value = module.postgres_automation.default_privileges
 }

 output "schema_access" {
+  description = "Schema-level access grants"
   value = module.postgres_automation.schema_access
 }

 output "table_access" {
+  description = "Table-level access grants"
   value = module.postgres_automation.table_access
 }

As per coding guidelines for Terraform files, Cloud Posse best practices recommend documenting all outputs with meaningful descriptions.

examples/llm_chat_app/variables.tf (1)

39-45: Add description to the databases variable.

The databases variable lacks a description, unlike other variables in this file.

♻️ Suggested fix
 variable "databases" {
   type = list(object({
     name             = string
     connection_limit = number
   }))
-  default = []
+  default     = []
+  description = "List of PostgreSQL databases to create with their connection limits."
 }
examples/llm_chat_app/main.tf (2)

40-169: Consider using for_each to reduce repetition.

The RW and RO group role grants follow identical patterns across object types. While the current explicit approach aids readability in example code, using for_each with a local map could reduce maintenance burden.

♻️ Example pattern using locals and for_each
locals {
  database = "llm_service"
  
  app_schema_grants = {
    rw_schema    = { role = "role_service_rw", object_type = "schema", privileges = ["USAGE"] }
    rw_tables    = { role = "role_service_rw", object_type = "table", privileges = ["SELECT", "INSERT", "UPDATE", "DELETE", "TRUNCATE"] }
    rw_sequences = { role = "role_service_rw", object_type = "sequence", privileges = ["USAGE", "SELECT", "UPDATE"] }
    ro_schema    = { role = "role_service_ro", object_type = "schema", privileges = ["USAGE"] }
    ro_tables    = { role = "role_service_ro", object_type = "table", privileges = ["SELECT"] }
    ro_sequences = { role = "role_service_ro", object_type = "sequence", privileges = ["USAGE", "SELECT"] }
  }
}

resource "postgresql_grant" "app_grants" {
  for_each    = local.app_schema_grants
  database    = local.database
  role        = each.value.role
  schema      = "app"
  object_type = each.value.object_type
  privileges  = each.value.privileges
  depends_on  = [module.postgres_automation, postgresql_schema.app]
}

This is optional—the explicit approach is perfectly valid for example code where clarity is prioritized over DRY.


1-566: Hardcoded database name could use a local variable.

The string "llm_service" appears 50+ times throughout this file. Using a local variable would make it easier to adapt this example for different database names.

♻️ Suggested pattern
locals {
  database_name = "llm_service"
}

# Then use local.database_name instead of "llm_service"

Comment thread examples/llm_chat_app/1_apply_terraform.sh Outdated
Comment thread examples/llm_chat_app/RUN_ALL_TESTS.sh Outdated
Comment thread examples/llm_chat_app/run_tests.sh Outdated
Comment thread examples/llm_chat_app/run_tests.sh Outdated
Comment thread examples/llm_chat_app/run_tests.sh
Comment thread examples/llm_chat_app/TEST_INSTRUCTIONS.md
Comment thread examples/llm_chat_app/TEST_INSTRUCTIONS.md
Comment thread examples/llm_chat_app/TEST_INSTRUCTIONS.md Outdated
Comment thread examples/llm_chat_app/TEST_INSTRUCTIONS.md Outdated
@westonplatter westonplatter requested a review from a team January 30, 2026 00:12
@westonplatter
Copy link
Copy Markdown
Member Author

@coderabbitai please reeview

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 30, 2026

@westonplatter, I'll review the changes in this PR for you.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 30, 2026

Caution

Docstrings generation - FAILED

No docstrings were generated.

@westonplatter
Copy link
Copy Markdown
Member Author

@coderabbitai generate unit tests

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@examples/llm_chat_app/2_create_test_objects.sh`:
- Around line 4-53: The psql invocation that runs the here-doc (the line
starting with "PGUSER=service_migrator PGPASSWORD=demo-password-migrator psql
<<'EOF'") does not pass ON_ERROR_STOP, so SQL errors inside the here-doc can be
ignored despite set -e; update that psql command to include the flag -v
ON_ERROR_STOP=1 so psql exits non‑zero on the first SQL error and the script
fails as expected.

In `@examples/llm_chat_app/TEST_INSTRUCTIONS.md`:
- Around line 89-95: Replace the incorrect password string
"demo-password-migration" with the canonical "demo-password-migrator" wherever
the `service_migrator` role is documented or used; specifically update the table
entry for `service_migrator` in TEST_INSTRUCTIONS.md and the occurrences in
run_tests.sh (the script invocations that set or reference the
`service_migrator` password) so the documented and runtime values match
`demo-password-migrator`.
🧹 Nitpick comments (1)
examples/llm_chat_app/main.tf (1)

40-566: Consider DRYing repetitive grant/default‑privilege blocks with for_each.

Many grant/default‑privilege resources are structurally identical. A local map + for_each would reduce drift and maintenance overhead.

♻️ Example refactor pattern (apply similarly elsewhere)
+locals {
+  app_rw_grants = {
+    schema   = { object_type = "schema",   privileges = ["USAGE"] }
+    table    = { object_type = "table",    privileges = ["SELECT", "INSERT", "UPDATE", "DELETE"] }
+    sequence = { object_type = "sequence", privileges = ["USAGE", "SELECT", "UPDATE"] }
+  }
+}
+
+resource "postgresql_grant" "rw_app" {
+  for_each    = local.app_rw_grants
+  database    = "llm_service"
+  role        = "role_service_rw"
+  schema      = "app"
+  object_type = each.value.object_type
+  privileges  = each.value.privileges
+  depends_on  = [module.postgres_automation, postgresql_schema.app]
+}
-
-resource "postgresql_grant" "rw_app_schema" {
-  database    = "llm_service"
-  role        = "role_service_rw"
-  schema      = "app"
-  object_type = "schema"
-  privileges  = ["USAGE"]
-
-  depends_on = [module.postgres_automation, postgresql_schema.app]
-}
-
-resource "postgresql_grant" "rw_app_tables" {
-  database    = "llm_service"
-  role        = "role_service_rw"
-  schema      = "app"
-  object_type = "table"
-  privileges  = ["SELECT", "INSERT", "UPDATE", "DELETE"]
-
-  depends_on = [module.postgres_automation, postgresql_schema.app]
-}
-
-resource "postgresql_grant" "rw_app_sequences" {
-  database    = "llm_service"
-  role        = "role_service_rw"
-  schema      = "app"
-  object_type = "sequence"
-  privileges  = ["USAGE", "SELECT", "UPDATE"]
-
-  depends_on = [module.postgres_automation, postgresql_schema.app]
-}

Comment on lines +4 to +53
set -e

export PGHOST=localhost
export PGPORT=5432
export PGDATABASE=llm_service

echo "=============================================="
echo "Creating Test Objects"
echo "=============================================="
echo ""

PGUSER=service_migrator PGPASSWORD=demo-password-migrator psql <<'EOF'
-- Switch to group role so objects are owned by role_service_migration
-- This ensures default privileges apply correctly
SET ROLE role_service_migration;

-- Create test table in app schema
CREATE TABLE IF NOT EXISTS app.test_users (
id SERIAL PRIMARY KEY,
name TEXT NOT NULL
);
INSERT INTO app.test_users (name) VALUES ('test') ON CONFLICT DO NOTHING;

-- Create test view in app schema
CREATE OR REPLACE VIEW app.test_users_view AS SELECT * FROM app.test_users;

-- Create test function in app schema
CREATE OR REPLACE FUNCTION app.test_func() RETURNS integer
LANGUAGE sql SECURITY INVOKER
AS $$ SELECT 1; $$;

-- Create test table in ref_data schemas
CREATE TABLE IF NOT EXISTS ref_data_pipeline_abc.test_ref (
id SERIAL PRIMARY KEY,
value TEXT
);
INSERT INTO ref_data_pipeline_abc.test_ref (value) VALUES ('abc') ON CONFLICT DO NOTHING;

CREATE TABLE IF NOT EXISTS ref_data_pipeline_xyz.test_ref (
id SERIAL PRIMARY KEY,
value TEXT
);
INSERT INTO ref_data_pipeline_xyz.test_ref (value) VALUES ('xyz') ON CONFLICT DO NOTHING;

-- Create views in ref_data schemas
CREATE OR REPLACE VIEW ref_data_pipeline_abc.test_ref_view AS SELECT * FROM ref_data_pipeline_abc.test_ref;
CREATE OR REPLACE VIEW ref_data_pipeline_xyz.test_ref_view AS SELECT * FROM ref_data_pipeline_xyz.test_ref;

SELECT 'Test objects created successfully!' AS result;
EOF
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n examples/llm_chat_app/2_create_test_objects.sh

Repository: masterpointio/terraform-postgres-config-dbs-users-roles

Length of output: 2381


🌐 Web query:

psql ON_ERROR_STOP behavior exit code sql errors

💡 Result:

psql’s ON_ERROR_STOP controls whether psql keeps going after an error or stops immediately.

Behavior

  • Default (ON_ERROR_STOP off): psql reports SQL (or meta-command) errors but continues processing subsequent commands. (postgresql.org)
  • With ON_ERROR_STOP=on (or \set ON_ERROR_STOP on):
    • Non-interactive/script mode (-f, stdin, etc.): psql terminates immediately when the first error occurs. (postgresql.org)
    • Interactive mode: psql stops processing the current batch and returns you to the prompt. (postgresql.org)

Exit codes (what $? / process status means)

psql uses these exit statuses:

  • 0: finished normally
  • 1: fatal error in psql itself (or other “fatal” conditions)
  • 2: connection to server failed and session was not interactive
  • 3: an error occurred while running a script and ON_ERROR_STOP was set (superscript.com)

Practical implication

If you want CI/shell scripts to fail on the first SQL error, use e.g.

  • psql -v ON_ERROR_STOP=1 -f script.sql
  • or inside scripts: \set ON_ERROR_STOP on (postgresql.org)

Citations:


Add ON_ERROR_STOP to fail the script on SQL errors.

The set -e at the top of the script will only catch non-zero exit codes from psql. Without ON_ERROR_STOP=1, SQL errors in the here-doc don't cause psql to exit with a non-zero status—it continues executing and exits 0, masking failures. Add -v ON_ERROR_STOP=1 to ensure the script fails on the first SQL error.

🛠️ Proposed fix
-PGUSER=service_migrator PGPASSWORD=demo-password-migrator psql <<'EOF'
+PGUSER=service_migrator PGPASSWORD=demo-password-migrator psql -v ON_ERROR_STOP=1 <<'EOF'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
set -e
export PGHOST=localhost
export PGPORT=5432
export PGDATABASE=llm_service
echo "=============================================="
echo "Creating Test Objects"
echo "=============================================="
echo ""
PGUSER=service_migrator PGPASSWORD=demo-password-migrator psql <<'EOF'
-- Switch to group role so objects are owned by role_service_migration
-- This ensures default privileges apply correctly
SET ROLE role_service_migration;
-- Create test table in app schema
CREATE TABLE IF NOT EXISTS app.test_users (
id SERIAL PRIMARY KEY,
name TEXT NOT NULL
);
INSERT INTO app.test_users (name) VALUES ('test') ON CONFLICT DO NOTHING;
-- Create test view in app schema
CREATE OR REPLACE VIEW app.test_users_view AS SELECT * FROM app.test_users;
-- Create test function in app schema
CREATE OR REPLACE FUNCTION app.test_func() RETURNS integer
LANGUAGE sql SECURITY INVOKER
AS $$ SELECT 1; $$;
-- Create test table in ref_data schemas
CREATE TABLE IF NOT EXISTS ref_data_pipeline_abc.test_ref (
id SERIAL PRIMARY KEY,
value TEXT
);
INSERT INTO ref_data_pipeline_abc.test_ref (value) VALUES ('abc') ON CONFLICT DO NOTHING;
CREATE TABLE IF NOT EXISTS ref_data_pipeline_xyz.test_ref (
id SERIAL PRIMARY KEY,
value TEXT
);
INSERT INTO ref_data_pipeline_xyz.test_ref (value) VALUES ('xyz') ON CONFLICT DO NOTHING;
-- Create views in ref_data schemas
CREATE OR REPLACE VIEW ref_data_pipeline_abc.test_ref_view AS SELECT * FROM ref_data_pipeline_abc.test_ref;
CREATE OR REPLACE VIEW ref_data_pipeline_xyz.test_ref_view AS SELECT * FROM ref_data_pipeline_xyz.test_ref;
SELECT 'Test objects created successfully!' AS result;
EOF
set -e
export PGHOST=localhost
export PGPORT=5432
export PGDATABASE=llm_service
echo "=============================================="
echo "Creating Test Objects"
echo "=============================================="
echo ""
PGUSER=service_migrator PGPASSWORD=demo-password-migrator psql -v ON_ERROR_STOP=1 <<'EOF'
-- Switch to group role so objects are owned by role_service_migration
-- This ensures default privileges apply correctly
SET ROLE role_service_migration;
-- Create test table in app schema
CREATE TABLE IF NOT EXISTS app.test_users (
id SERIAL PRIMARY KEY,
name TEXT NOT NULL
);
INSERT INTO app.test_users (name) VALUES ('test') ON CONFLICT DO NOTHING;
-- Create test view in app schema
CREATE OR REPLACE VIEW app.test_users_view AS SELECT * FROM app.test_users;
-- Create test function in app schema
CREATE OR REPLACE FUNCTION app.test_func() RETURNS integer
LANGUAGE sql SECURITY INVOKER
AS $$ SELECT 1; $$;
-- Create test table in ref_data schemas
CREATE TABLE IF NOT EXISTS ref_data_pipeline_abc.test_ref (
id SERIAL PRIMARY KEY,
value TEXT
);
INSERT INTO ref_data_pipeline_abc.test_ref (value) VALUES ('abc') ON CONFLICT DO NOTHING;
CREATE TABLE IF NOT EXISTS ref_data_pipeline_xyz.test_ref (
id SERIAL PRIMARY KEY,
value TEXT
);
INSERT INTO ref_data_pipeline_xyz.test_ref (value) VALUES ('xyz') ON CONFLICT DO NOTHING;
-- Create views in ref_data schemas
CREATE OR REPLACE VIEW ref_data_pipeline_abc.test_ref_view AS SELECT * FROM ref_data_pipeline_abc.test_ref;
CREATE OR REPLACE VIEW ref_data_pipeline_xyz.test_ref_view AS SELECT * FROM ref_data_pipeline_xyz.test_ref;
SELECT 'Test objects created successfully!' AS result;
EOF
🤖 Prompt for AI Agents
In `@examples/llm_chat_app/2_create_test_objects.sh` around lines 4 - 53, The psql
invocation that runs the here-doc (the line starting with
"PGUSER=service_migrator PGPASSWORD=demo-password-migrator psql <<'EOF'") does
not pass ON_ERROR_STOP, so SQL errors inside the here-doc can be ignored despite
set -e; update that psql command to include the flag -v ON_ERROR_STOP=1 so psql
exits non‑zero on the first SQL error and the script fails as expected.

Comment on lines +89 to +95
| Role | Password | Access Level |
| --------------------- | --------------------------- | ------------------------- |
| `service_migrator` | `demo-password-migration` | DDL + DML on all schemas |
| `service_fastapi_rw` | `demo-password-fastapi-rw` | DML on app schema only |
| `service_fastapi_ro` | `demo-password-fastapi-ro` | SELECT on app schema only |
| `service_pipeline_rw` | `demo-password-pipeline-rw` | DML on all schemas |
| `service_pipeline_ro` | `demo-password-pipeline-ro` | SELECT on all schemas |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's find the files mentioned in the review
find . -name "TEST_INSTRUCTIONS.md" -o -name "2_create_test_objects.sh" -o -name "fixtures.auto.tfvars" | head -20

Repository: masterpointio/terraform-postgres-config-dbs-users-roles

Length of output: 277


🏁 Script executed:

# Search for all references to these password values
rg -n "demo-password" --no-heading -g '!**/.terraform/**' | head -50

Repository: masterpointio/terraform-postgres-config-dbs-users-roles

Length of output: 4330


Align migrator password to canonical value across docs and scripts.

The canonical password for service_migrator is demo-password-migrator (from fixtures.auto.tfvars). However, TEST_INSTRUCTIONS.md documents demo-password-migration, and run_tests.sh also uses the incorrect value. This mismatch will cause authentication failures. Update both files to use demo-password-migrator.

💡 Changes needed

In TEST_INSTRUCTIONS.md (line 91):

-| `service_migrator`    | `demo-password-migration`   | DDL + DML on all schemas  |
+| `service_migrator`    | `demo-password-migrator`    | DDL + DML on all schemas  |

In run_tests.sh (lines 34, 78, 126, 136): Update demo-password-migration to demo-password-migrator

🤖 Prompt for AI Agents
In `@examples/llm_chat_app/TEST_INSTRUCTIONS.md` around lines 89 - 95, Replace the
incorrect password string "demo-password-migration" with the canonical
"demo-password-migrator" wherever the `service_migrator` role is documented or
used; specifically update the table entry for `service_migrator` in
TEST_INSTRUCTIONS.md and the occurrences in run_tests.sh (the script invocations
that set or reference the `service_migrator` password) so the documented and
runtime values match `demo-password-migrator`.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 30, 2026

✅ Unit tests generated successfully!

Generated 10 test files. The changes have been created in this pull request: View PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 30, 2026

✅ Actions performed

Unit Test Generation started...


Unit tests generated successfully.

@westonplatter westonplatter changed the title feat: add dependencies to role creation process feat!: add dependencies to role creation process Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant