[PLAT-446] fix: make database name configurable via values#41
[PLAT-446] fix: make database name configurable via values#41AntTheLimey wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughDynamically generate PostgreSQL pg_hba and pg_ident rules from bootstrap.initdb per cluster node; replace hardcoded DB name with a Helm dig expression; add a Bash template test and CI step; remove the "App database name" doc subsection and update installation note about certificate name conflicts. Changes
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 unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 (2)
scripts/test-template.sh (2)
64-84: "Custom database" scenario is missing apg_identassertion.The default scenario asserts
local postgres appin pg_ident (test 5). The custom-database scenario changesdatabase=mydbwhileownerremains"app"fromvalues.yamldefaults — so the cluster template still emitslocal postgres appin pg_ident. There is no test verifying this rule is present (or absent of any stale entry) in the custom-database case, creating a gap between the tested scenario and the tested identity-map rules.✅ Suggested addition
assert_contains "pg_hba still includes streaming_replica rule" \ "$CUSTOM" \ "hostssl all streaming_replica all cert map=cnpg_streaming_replica" + +assert_contains "pg_ident still references default app owner with custom db" \ + "$CUSTOM" \ + "local postgres app"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-template.sh` around lines 64 - 84, Add an assertion in the "Custom database name (mydb)" test block to verify the pg_ident mapping still contains the expected identity rule; specifically, after computing CUSTOM (the helm template output) use the same assert function (assert_contains) to check that the pg_ident section includes the string "local postgres app" so we ensure the identity-map didn't change when database was set to mydb; locate the CUSTOM variable and existing assertions in scripts/test-template.sh and insert the assert_contains referencing CUSTOM, pg_ident (or piping CUSTOM through grep to the pg_ident block) and the literal "local postgres app".
42-42:2>/dev/nullswallows helm diagnostics — drop it or redirect to a temp file.With
set -ein effect, a non-zero exit fromhelm templatealready stops the script. Suppressing stderr means that when a rendering failure does occur, CI produces no diagnostic output, making the failure opaque to the engineer on call. Sincehelm lintruns before this step and guards against template errors, the suppression provides limited value.🔧 Proposed fix
-DEFAULT=$(helm template pgedge "$CHART_DIR" -f "$VALUES" 2>/dev/null) +DEFAULT=$(helm template pgedge "$CHART_DIR" -f "$VALUES") -CUSTOM=$(helm template pgedge "$CHART_DIR" -f "$VALUES" \ - --set pgEdge.clusterSpec.bootstrap.initdb.database=mydb 2>/dev/null) +CUSTOM=$(helm template pgedge "$CHART_DIR" -f "$VALUES" \ + --set pgEdge.clusterSpec.bootstrap.initdb.database=mydb) -CUSTOM_OWNER=$(helm template pgedge "$CHART_DIR" -f "$VALUES" \ - --set pgEdge.clusterSpec.bootstrap.initdb.database=mydb \ - --set pgEdge.clusterSpec.bootstrap.initdb.owner=myuser 2>/dev/null) +CUSTOM_OWNER=$(helm template pgedge "$CHART_DIR" -f "$VALUES" \ + --set pgEdge.clusterSpec.bootstrap.initdb.database=mydb \ + --set pgEdge.clusterSpec.bootstrap.initdb.owner=myuser)Also applies to: 67-68, 89-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-template.sh` at line 42, The helm template invocations are suppressing stderr via "2>/dev/null" which hides useful diagnostics; locate the assignments using the helm template command (e.g., the DEFAULT variable assignment that runs: helm template pgedge "$CHART_DIR" -f "$VALUES") and remove the stderr redirection so errors are visible, or alternatively redirect stderr to a temporary logfile and surface its contents on failure; apply the same change to the other helm template occurrences in the script that use "2>/dev/null" so CI failures produce diagnostic output.
🤖 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/test-template.sh`:
- Around line 78-80: The grep pipelines used as haystacks for
assert_not_contains (e.g., the command substitutions using "$(echo "$CUSTOM" |
grep -F 'hostssl' | grep -F 'pgedge' | grep -F 'cert' | grep -vF
'streaming_replica')") can produce an empty string when an intermediate grep
yields no matches under pipefail; append "|| true" to each pipeline in the
command substitutions so they never fail and always return a (possibly empty)
string to assert_not_contains; apply the same change to the other similar
pipelines referenced around the assert_not_contains calls for the CUSTOM
variable (the blocks at the other mentioned locations).
In `@templates/cluster.yaml`:
- Around line 33-46: The template can emit duplicate pg_hba/pg_ident lines when
$dbOwner is "pgedge" or "admin"; modify the generation of $generatedHba and
$generatedIdent so the owner-specific entries are only appended when $dbOwner is
not one of the reserved names (e.g. "pgedge" or "admin"). In practice, change
the list building for $generatedHba (the printf "hostssl %s %s ..." entry) and
for $generatedIdent (the printf "local postgres %s" entry) to be conditional on
$dbOwner not being in ["pgedge","admin"], leaving the concat into
$merged.postgresql.pg_hba and pg_ident unchanged.
---
Nitpick comments:
In `@scripts/test-template.sh`:
- Around line 64-84: Add an assertion in the "Custom database name (mydb)" test
block to verify the pg_ident mapping still contains the expected identity rule;
specifically, after computing CUSTOM (the helm template output) use the same
assert function (assert_contains) to check that the pg_ident section includes
the string "local postgres app" so we ensure the identity-map didn't change when
database was set to mydb; locate the CUSTOM variable and existing assertions in
scripts/test-template.sh and insert the assert_contains referencing CUSTOM,
pg_ident (or piping CUSTOM through grep to the pg_ident block) and the literal
"local postgres app".
- Line 42: The helm template invocations are suppressing stderr via
"2>/dev/null" which hides useful diagnostics; locate the assignments using the
helm template command (e.g., the DEFAULT variable assignment that runs: helm
template pgedge "$CHART_DIR" -f "$VALUES") and remove the stderr redirection so
errors are visible, or alternatively redirect stderr to a temporary logfile and
surface its contents on failure; apply the same change to the other helm
template occurrences in the script that use "2>/dev/null" so CI failures produce
diagnostic output.
AntTheLimey
force-pushed
the
fix/configurable-db-name
branch
from
48a8340 to
6f86b3f
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@templates/cluster.yaml`:
- Around line 37-39: The conditional that prevents adding duplicate hostssl
entries when $dbOwner is "pgedge" or "admin" is already correctly implemented
for $generatedHba (the block using append with printf for hostssl %s %s
0.0.0.0/0 cert); keep this guard as-is and ensure the same pattern/guard is also
present for the pg_ident-related block (the conditional around the pg_ident
append at lines noted previously) so both $generatedHba and pg_ident logic avoid
duplicate entries for those owners.
- Use dig for nil-safe traversal of bootstrap.initdb.database in init-spock-job template instead of hardcoding DB_NAME=app - Generate pg_hba entries from database name and owner in cluster template so they stay in sync automatically - Generate pg_ident entries from owner in cluster template - Append user-supplied pg_hba/pg_ident entries after generated rules so custom entries are preserved - Remove hardcoded pg_hba and pg_ident from values.yaml defaults - Remove app database name limitation from docs - Add 14 template assertion tests for database name configuration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AntTheLimey
force-pushed
the
fix/configurable-db-name
branch
from
6f86b3f to
f552ff9
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test-template.sh`:
- Around line 42-121: Add a new test case that injects one custom pg_hba and one
custom pg_ident entry and asserts each custom line appears after the generated
line: call helm template similarly to CUSTOM/CUSTOM_OWNER (e.g., create
CUSTOM_ORDER) using --set to add a user-supplied pg_hba and pg_ident entry, then
locate the generated lines (grep for the generated "hostssl ...
streaming_replica" and "local postgres app" or their per-db variants) and the
custom lines (your custom hostssl and custom local postgres entries) with grep
-n to get line numbers and assert numerically that generated_line_number <
custom_line_number; reuse existing helpers (assert_contains/assert_not_contains)
only for existence checks, and perform the ordering check with a small numeric
comparison so the test ensures append-order of user-supplied entries after
generated entries.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/test.yamldocs/limitations.mdscripts/test-template.shtemplates/cluster.yamltemplates/init-spock-job.yamlvalues.yaml
💤 Files with no reviewable changes (1)
- docs/limitations.md
🚧 Files skipped from review as they are similar to previous changes (1)
- templates/cluster.yaml
Adds assert_after helper and a test scenario verifying that user-supplied pg_hba/pg_ident entries appear after generated ones, as guaranteed by the concat ordering in cluster.yaml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/test-template.sh (1)
146-148: Prefer--set-stringfor literalpg_hbaandpg_identconfiguration entries.According to Helm documentation,
--setapplies YAML-type coercion to values, while--set-stringforces values to remain strings. For configuration rules likepg_hbaandpg_ident, using--set-stringmakes intent explicit and ensures these remain literal strings across Helm versions.♻️ Suggested change
CUSTOM_ORDER=$(helm template pgedge "$CHART_DIR" -f "$VALUES" \ - --set 'pgEdge.clusterSpec.postgresql.pg_hba[0]=hostssl app custom_user 0.0.0.0/0 cert' \ - --set 'pgEdge.clusterSpec.postgresql.pg_ident[0]=local postgres custom_user' 2>"$STDERR_LOG") || { + --set-string 'pgEdge.clusterSpec.postgresql.pg_hba[0]=hostssl app custom_user 0.0.0.0/0 cert' \ + --set-string 'pgEdge.clusterSpec.postgresql.pg_ident[0]=local postgres custom_user' 2>"$STDERR_LOG") || {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-template.sh` around lines 146 - 148, The helm template invocation that sets pg_hba and pg_ident entries should use --set-string to force literal strings; update the CUSTOM_ORDER command where helm template pgedge ... --set 'pgEdge.clusterSpec.postgresql.pg_hba[0]=hostssl app custom_user 0.0.0.0/0 cert' and --set 'pgEdge.clusterSpec.postgresql.pg_ident[0]=local postgres custom_user' to use --set-string for those two keys (pgEdge.clusterSpec.postgresql.pg_hba[0] and pgEdge.clusterSpec.postgresql.pg_ident[0]) so the values are treated as plain strings rather than being YAML-coerced.
🤖 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/test-template.sh`:
- Around line 146-148: The helm template invocation that sets pg_hba and
pg_ident entries should use --set-string to force literal strings; update the
CUSTOM_ORDER command where helm template pgedge ... --set
'pgEdge.clusterSpec.postgresql.pg_hba[0]=hostssl app custom_user 0.0.0.0/0 cert'
and --set 'pgEdge.clusterSpec.postgresql.pg_ident[0]=local postgres custom_user'
to use --set-string for those two keys (pgEdge.clusterSpec.postgresql.pg_hba[0]
and pgEdge.clusterSpec.postgresql.pg_ident[0]) so the values are treated as
plain strings rather than being YAML-coerced.
mmols
left a comment
mmols
left a comment
There was a problem hiding this comment.
I think this is in the right direction, but let's discuss the UX implications of the templating together and figure out the path forward.
| - name: Chart lint | ||
| run: ct lint --chart-dirs . --charts . | ||
|
|
||
| - name: Template tests |
There was a problem hiding this comment.
Let's convert this to a unit test (introduced after this PR was opened) and remove this script approach.
The things being tested in the shell script align with the other things the unit tests are testing.
| {{- $_ := set $merged.postgresql.parameters "snowflake.node" $ordStr -}} | ||
| {{- $_ := set $merged.postgresql.parameters "lolor.node" $ordStr -}} | ||
|
|
||
| {{- /* generate pg_hba and pg_ident from database name so they stay in sync */ -}} |
There was a problem hiding this comment.
We've lost the ability to fully override pg_hba and pg_ident with this change. This templating lets you append new values, but I think we should allow users to do full overrides to meet requirements in their own environments.
It seems like there are potential paths here:
- Remove this templating and document that changing the database name requires:
- setting bootstrap.initdb.database
- overriding pg_hba / pg_ident lines
- Add logic which fully passes through pg_hba and pg_ident if they are set by the user (rather than just appending). As part of this, the templating logic in here could also be simplified a bit to remove some conditionals.
There was a problem hiding this comment.
Another thought: If we are adjusting the templating logic in here, we should probably cover anything that the user can override that might show up here, not just the app database name.
| pg_ident: | ||
| - local postgres admin | ||
| - local postgres app | ||
| # pg_hba and pg_ident are auto-generated by the cluster template from |
There was a problem hiding this comment.
Using helm values is common to understand what the defaults are. We should duplicate the clear defaults into comments in here rather than pointing to the template.
2 participants
Summary
digfor nil-safe lookup ofbootstrap.initdb.databasein init-spock-job template instead of hardcodingDB_NAME=apppg_hbaentries from database name and owner in cluster template so they stay in sync automaticallypg_idententries from owner in cluster templatepg_hba/pg_idententries after generated rules so custom entries are preservedpg_hbaandpg_identfromvalues.yamldefaultsTest plan
helm templatewith default values renders identical output to mainhelm templatewith--set pgEdge.clusterSpec.bootstrap.initdb.database=mydbrenders correct DB_NAME, pg_hba, and pg_identhelm templatewith custom database + custom owner renders correct owner in pg_hba and pg_identhelm lint --strictpassesscripts/test-template.sh)database=mydb— init-spock completes, Spock subscriptions replicate, bidirectional writes confirmed🤖 Generated with Claude Code