feat: make database name, owner, and admin role configurable#54
Merged
Conversation
📝 WalkthroughWalkthroughAdds a configurable Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
mmols
changed the title
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 19 duplication
Metric Results Duplication 19
TIP This summary will be updated as you push new changes. Give us 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/configuration.md.gotmpl`:
- Around line 77-78: The docs currently hardcode per-node password secret names
as `pgedge-n#-app` and `pgedge-n#-admin` which conflicts with configurable role
names; update the table rows for the "app" (owner) and "admin" entries so the
secret name examples reflect the configurable role (e.g., `pgedge-n#-<role>` or
explicitly reference the configuration keys `initdb.owner` and
`pgEdge.adminUser`) and mention the actual role name will be substituted from
those settings rather than being fixed to "app" or "admin".
In `@templates/cert-manager-cert-ca.yaml`:
- Line 110: The templated commonName values in
templates/cert-manager-cert-ca.yaml (the commonName entries that use
.Values.pgEdge.adminUser and the other commonName at the second occurrence)
should be wrapped with the Helm quote filter to avoid YAML parsing edge cases;
locate the commonName template uses and apply the quote filter to those .Values
references so user-provided strings with special characters or YAML-ambiguous
content are rendered safely.
In `@templates/init-spock-job.yaml`:
- Around line 25-27: The DB_NAME and ADMIN_USER env values are not quoted;
update the template to use the Helm quote filter on both values so they match
the style of APP_NAME and INIT_SPOCK_TIMEOUT — locate the DB_NAME env entry
(value using .Values.pgEdge.clusterSpec.bootstrap.initdb.database) and the
ADMIN_USER env entry (value using .Values.pgEdge.adminUser) and change them to
use the | quote filter.
In `@test/integration/install_test.go`:
- Around line 272-297: The two tests custom_admin_can_connect and
custom_owner_can_connect race with cert-manager; before calling ConnectWithCert
you should wait for the corresponding certificate to be issued/ready. Add a
readiness wait for "admin-client-cert" before the ConnectWithCert call in
custom_admin_can_connect and for "app-client-cert" in custom_owner_can_connect
(e.g. call wait.ForCertificateReady(testKube, "admin-client-cert", timeout) and
wait.ForCertificateReady(testKube, "app-client-cert", timeout) or the equivalent
testKube.WaitForCertificateReady helper) and fail the test with a clear message
if the wait errors.
In `@test/unit/job_test.go`:
- Around line 164-172: The test currently slices hba with hba[:3] which will
panic if fewer than 3 entries are present; modify the assertion in the test (the
hba variable usage in the test function) to guard against short slices by
computing a safe iteration bound (e.g., n := min(3, len(hba))) and iterate up to
n, and if len(hba) < 3 call t.Fatalf or t.Errorf with a clear message so missing
entries produce a test failure instead of a panic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2b912f2-a076-4ca4-9908-89ddc66a75a2
📒 Files selected for processing (15)
README.mdREADME.md.gotmpldocs/configuration.mddocs/configuration.md.gotmpldocs/limitations.mdinternal/config/config.gointernal/config/config_test.gotemplates/cert-manager-cert-ca.yamltemplates/init-spock-job.yamltest/integration/install_test.gotest/integration/testdata/custom-database-values.yamltest/pkg/kube/kube.gotest/unit/job_test.gotest/unit/testdata/custom-database-values.yamlvalues.yaml
💤 Files with no reviewable changes (1)
- docs/limitations.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/job_test.go (1)
91-142: Consider extracting a shared helper for Job container env parsing.
NestedSlice(...containers...)and env-map extraction are repeated; a small test helper would reduce duplication and make failures easier to interpret.♻️ Optional refactor sketch
+func getJobEnvMap(t *testing.T, job unstructured.Unstructured) map[string]string { + t.Helper() + containers, found, _ := unstructured.NestedSlice(job.Object, "spec", "template", "spec", "containers") + if !found || len(containers) == 0 { + t.Fatal("no containers found in job") + } + container := containers[0].(map[string]interface{}) + envVars, _, _ := unstructured.NestedSlice(container, "env") + out := map[string]string{} + for _, e := range envVars { + env := e.(map[string]interface{}) + name, nOK := env["name"].(string) + val, vOK := env["value"].(string) + if nOK && vOK { + out[name] = val + } + } + return out +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/job_test.go` around lines 91 - 142, Extract the duplicated container/env parsing into a small test helper (e.g., getJobContainerEnvMap or extractContainerEnvMap) that takes the rendered objects or a Job unstructured object and returns a map[string]string of env names->values; replace the duplicated blocks in TestInitSpockJob and TestInitSpockJobCustomDBName to call this helper instead of repeating unstructured.NestedSlice(...,"spec","template","spec","containers") and the envMap population loop, preserving the existing error checks (no containers found) inside the helper so callers can simply assert on the returned map values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/job_test.go`:
- Around line 91-142: Extract the duplicated container/env parsing into a small
test helper (e.g., getJobContainerEnvMap or extractContainerEnvMap) that takes
the rendered objects or a Job unstructured object and returns a
map[string]string of env names->values; replace the duplicated blocks in
TestInitSpockJob and TestInitSpockJobCustomDBName to call this helper instead of
repeating unstructured.NestedSlice(...,"spec","template","spec","containers")
and the envMap population loop, preserving the existing error checks (no
containers found) inside the helper so callers can simply assert on the returned
map values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 476ed851-2631-4554-9b70-d549992cad67
📒 Files selected for processing (7)
changes/unreleased/Added-20260416-155036.yamldocs/configuration.mddocs/configuration.md.gotmpltemplates/cert-manager-cert-ca.yamltemplates/init-spock-job.yamltest/integration/install_test.gotest/unit/job_test.go
✅ Files skipped from review due to trivial changes (2)
- changes/unreleased/Added-20260416-155036.yaml
- docs/configuration.md.gotmpl
🚧 Files skipped from review as they are similar to previous changes (2)
- templates/init-spock-job.yaml
- docs/configuration.md
rshoemaker
approved these changes
Apr 17, 2026
Contributor
rshoemaker
left a comment
rshoemaker
left a comment
There was a problem hiding this comment.
Looks good - couple minor questions/suggestions.
The init-spock job hardcoded DB_NAME=app and the admin user was hardcoded throughout templates and Go code. This made it impossible to customize the database name, owner, or admin role without forking. - Template DB_NAME and ADMIN_USER in the init-spock job from values - Template cert commonName for app and admin certs from values - Add pgEdge.adminUser value (default: admin) - Read ADMIN_USER env var in Go config with admin as default - Add users and authentication docs addressing #42 - Add unit and E2E tests for custom database/owner/admin overrides
- Quote DB_NAME, ADMIN_USER, and cert commonName template values - Use dynamic secret names in docs for configurable roles - Add certificate readiness gate before cert-auth E2E assertions - Guard hba[:3] slice against panic on short arrays
mmols
force-pushed
the
feat/PLAT-446/config-users-db
branch
from
d1b487f to
a7fb246
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/unit/job_test.go (1)
110-142: Consider extracting env parsing into a shared test helper.The env-to-map conversion pattern is likely reusable and will reduce repetition across Job env assertions.
♻️ Optional refactor
+func envMapFromContainer(t *testing.T, container map[string]interface{}) map[string]string { + t.Helper() + envVars, found, _ := unstructured.NestedSlice(container, "env") + if !found { + t.Fatal("expected env list in job container") + } + out := map[string]string{} + for _, e := range envVars { + env, ok := e.(map[string]interface{}) + if !ok { + continue + } + name, _ := env["name"].(string) + val, _ := env["value"].(string) + if name != "" { + out[name] = val + } + } + return out +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/job_test.go` around lines 110 - 142, Extract the env-to-map conversion in TestInitSpockJobCustomDBName into a shared test helper (e.g., envSliceToMap or parseEnvVarsToMap) that accepts the envVars slice (the variable currently built from unstructured.NestedSlice) and returns a map[string]string; then replace the inline loop that builds envMap in TestInitSpockJobCustomDBName with a call to that helper, and reuse this helper in other tests that perform similar env assertions (helpers should be placed in the same package test file or a _test_helpers.go file so TestInitSpockJobCustomDBName, renderTemplate, and filterByKind can call it).test/integration/install_test.go (1)
318-327: Use an exact boolean/row-count assertion for app DB absence.
strings.Contains(out, "1")is workable but loose. An exactEXISTS/COUNTcheck makes failures less ambiguous.✅ Suggested tightening
- out, err := testKube.ExecSQLWith(pod, - "SELECT 1 FROM pg_database WHERE datname = 'app';", "dbadmin", "mydb") + out, err := testKube.ExecSQLWith(pod, + "SELECT EXISTS (SELECT 1 FROM pg_database WHERE datname = 'app');", "dbadmin", "mydb") if err != nil { t.Fatalf("failed to query pg_database: %v", err) } - if strings.Contains(out, "1") { - t.Error("expected 'app' database to not exist, but it does") + if strings.TrimSpace(out) != "f" { + t.Errorf("expected app database to not exist, got: %q", out) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/install_test.go` around lines 318 - 327, The test uses a loose substring check on ExecSQLWith output to assert the absence of the "app" DB; change the SQL to a deterministic count or exists check (e.g., "SELECT COUNT(*) FROM pg_database WHERE datname = 'app';" or "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = 'app');"), call testKube.ExecSQLWith (same call site in the t.Run "default_app_database_does_not_exist" using getPodName("pgedge-n1")), parse the returned output into an integer/boolean, and assert exactly that the count == 0 or exists == false instead of using strings.Contains(out, "1").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/integration/install_test.go`:
- Around line 318-327: The test uses a loose substring check on ExecSQLWith
output to assert the absence of the "app" DB; change the SQL to a deterministic
count or exists check (e.g., "SELECT COUNT(*) FROM pg_database WHERE datname =
'app';" or "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = 'app');"),
call testKube.ExecSQLWith (same call site in the t.Run
"default_app_database_does_not_exist" using getPodName("pgedge-n1")), parse the
returned output into an integer/boolean, and assert exactly that the count == 0
or exists == false instead of using strings.Contains(out, "1").
In `@test/unit/job_test.go`:
- Around line 110-142: Extract the env-to-map conversion in
TestInitSpockJobCustomDBName into a shared test helper (e.g., envSliceToMap or
parseEnvVarsToMap) that accepts the envVars slice (the variable currently built
from unstructured.NestedSlice) and returns a map[string]string; then replace the
inline loop that builds envMap in TestInitSpockJobCustomDBName with a call to
that helper, and reuse this helper in other tests that perform similar env
assertions (helpers should be placed in the same package test file or a
_test_helpers.go file so TestInitSpockJobCustomDBName, renderTemplate, and
filterByKind can call it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53a15342-046f-467f-b22c-731d67825dd9
📒 Files selected for processing (16)
README.mdREADME.md.gotmplchanges/unreleased/Added-20260416-155036.yamldocs/configuration.mddocs/configuration.md.gotmpldocs/limitations.mdinternal/config/config.gointernal/config/config_test.gotemplates/cert-manager-cert-ca.yamltemplates/init-spock-job.yamltest/integration/install_test.gotest/integration/testdata/custom-database-values.yamltest/pkg/kube/kube.gotest/unit/job_test.gotest/unit/testdata/custom-database-values.yamlvalues.yaml
💤 Files with no reviewable changes (1)
- docs/limitations.md
✅ Files skipped from review due to trivial changes (8)
- README.md
- changes/unreleased/Added-20260416-155036.yaml
- README.md.gotmpl
- internal/config/config_test.go
- templates/cert-manager-cert-ca.yaml
- test/integration/testdata/custom-database-values.yaml
- docs/configuration.md.gotmpl
- test/unit/testdata/custom-database-values.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- values.yaml
- templates/init-spock-job.yaml
- internal/config/config.go
- docs/configuration.md
- test/pkg/kube/kube.go
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
2 participants
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
Makes the database name, owner, and admin role configurable via Helm values instead of being hardcoded to
app/admin. All defaults are unchanged — existing deployments see no difference.Changes
Templates
init-spock-job.yaml—DB_NAMEandADMIN_USERenv vars read from values instead of hardcodedcert-manager-cert-ca.yaml—commonNameforapp-client-certandadmin-client-certtemplated frominitdb.ownerandpgEdge.adminUserValues
pgEdge.adminUser(default:admin)Go code
config.go— readsADMIN_USERenv var withadminas defaultDocs
limitations.mdTests
TestCustomDatabaseInstall— deploys with custom database (mydb), owner (myuser), and admin (dbadmin), verifies cert auth for both custom admin and owner, spock initialization on custom databaseExecSQLAshelper for specifying user and database in psql callsBackwards compatibility
Verified via
helm templatediff — the only change with default values is the addition ofADMIN_USER: adminenv var in the init-spock job. All other rendered output is identical.Test plan
TestCustomDatabaseInstall— custom database, owner, and admin all connect and spock initializesAddresses #42.