Align dbscripts directory names with database names using underscores - #4149
Align dbscripts directory names with database names using underscores#4149indeewari wants to merge 1 commit into
Conversation
The dbscripts schema directories used kebab-case (runtime-transient, runtime-persistent) while the Postgres database names and config keys use underscores (runtime_transient, runtime_persistent). Because the two forms differed, the Helm quickstart schema-apply loop resolved directory paths that do not exist and silently left those two databases empty. Rename the two dbscripts directories and their SQLite files to underscores so the directory name matches the database name and config key for all four databases, and update every reference: build scripts, integration test config, Helm and OpenChoreo charts, the local-development compose source paths, and the integration-test CI action. Kubernetes Secret data keys and the container-internal compose mount aliases keep their kebab-case names, as those belong to separate naming domains. Fixes thunder-id#4120
📝 WalkthroughWalkthroughRuntime transient and persistent databases now use underscore-separated names across schemas, cleanup procedures, initialization scripts, server configuration, Helm deployments, local development, and integration-test resources. New SQLite and PostgreSQL persistence schemas and batched PostgreSQL cleanup procedures are included. ChangesRuntime database alignment
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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: 3
🧹 Nitpick comments (3)
backend/dbscripts/runtime_persistent/postgres-cleanup.sql (1)
73-76: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrevent concurrent cleanup runs from aborting prematurely.
If two cleanup runs overlap (e.g., a manual run overlaps with a scheduled run), both might
SELECTthe exact same batch ofctids. The first to execute theDELETEwill succeed, while the second will block. When the first commits, the second will wake up, find the rows already deleted, and reportv_deleted = 0, causing it to exit the loop prematurely even if there are more expired rows remaining.To allow concurrent runs to safely process different batches without blocking or aborting each other, append
FOR UPDATE SKIP LOCKEDto the subquery.🛠️ Proposed fix
DELETE FROM "REVOKED_TOKEN" WHERE ctid IN ( - SELECT ctid FROM "REVOKED_TOKEN" WHERE EXPIRY_TIME < v_now LIMIT p_batch_size + SELECT ctid FROM "REVOKED_TOKEN" WHERE EXPIRY_TIME < v_now LIMIT p_batch_size FOR UPDATE SKIP LOCKED );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/dbscripts/runtime_persistent/postgres-cleanup.sql` around lines 73 - 76, Update the REVOKED_TOKEN cleanup DELETE subquery to lock selected expired rows with FOR UPDATE SKIP LOCKED, ensuring overlapping cleanup runs process separate batches without blocking or exiting prematurely. Preserve the existing expiry filter and p_batch_size limit.backend/dbscripts/runtime_persistent/postgres.sql (1)
92-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStandardize timestamp types across the schema.
The
CONSENTtable usesTIMESTAMPTZandNOW(), whereas other tables in this schema (such asSSO_SESSIONandREVOKED_TOKEN) useTIMESTAMPandCURRENT_TIMESTAMP. Consider using a consistent timestamp type and default function throughout the schema. PreferringTIMESTAMPTZis generally recommended for PostgreSQL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/dbscripts/runtime_persistent/postgres.sql` around lines 92 - 102, Standardize the CONSENT table timestamp columns VALIDITY_TIME, CREATED_AT, and UPDATED_AT on TIMESTAMPTZ with CURRENT_TIMESTAMP defaults, replacing the inconsistent timestamp default usage while preserving the existing nullable and column constraints.backend/dbscripts/runtime_transient/postgres.sql (1)
90-113: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a
DEFAULTpartition toRUNTIME_STORE.
RUNTIME_STOREis list-partitioned byNAMESPACEwith noDEFAULTpartition. Any insert whoseNAMESPACEdoesn't match one of the 11 listed values will hard-fail with "no partition of relation found for row" rather than degrading gracefully. Given this PR exists specifically to fix a schema/name mismatch that silently broke runtime DB initialization, a missed partition update (per the comment at line 101-102, adding a namespace constant "REQUIRES adding a matching partition here") is the same class of easy-to-miss oversight, but this time it would surface as a hard runtime failure instead of a quiet compile-time miss.🛡️ Proposed fix: add a DEFAULT partition as a safety net
CREATE TABLE "RUNTIME_STORE_VP_STATE" PARTITION OF "RUNTIME_STORE" FOR VALUES IN ('vp:state'); + +-- Safety net for any NAMESPACE value not yet covered by an explicit partition above. +CREATE TABLE "RUNTIME_STORE_DEFAULT" PARTITION OF "RUNTIME_STORE" DEFAULT;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/dbscripts/runtime_transient/postgres.sql` around lines 90 - 113, Add a DEFAULT child partition for RUNTIME_STORE after the explicit namespace partitions, preserving all existing namespace-specific partitions. Ensure unmatched NAMESPACE values are routed to this fallback partition instead of failing inserts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/cmd/server/config/default.json`:
- Line 48: The SQLite filename rename must preserve existing databases by
migrating or detecting the hyphenated filenames and updating configured paths
consistently. Apply this to backend/cmd/server/config/default.json lines 48-48
and 87-87; preserve both databases in backend/cmd/server/deployment.yaml lines
27-27 and 45-45; document or automate PVC migration in install/helm/values.yaml
lines 311-311 and 377-377; preserve storage in
install/openchoreo/helm/charts/thunderid-component/templates/thunderid-component.yaml
lines 81-81 and 85-85; document or automate migration in its values.yaml lines
52-52 and 58-58 and install/openchoreo/helm/values.yaml lines 70-70 and 76-76;
and preserve both databases in
install/openchoreo/thunderid-oc-resourcetype/templates/thunderid-resourcetype.yaml
lines 418-418 and 434-434.
In `@backend/internal/system/database/provider/dbprovider.go`:
- Around line 42-44: Update the relevant SQLite deployment documentation to
describe the migration from runtime-transient.db and runtime-persistent.db to
runtime_transient.db and runtime_persistent.db, including updating configured
deployment paths to match the renamed files.
In
`@install/openchoreo/helm/charts/thunderid-component/templates/thunderid-release.yaml`:
- Around line 76-80: Update the DB_RUNTIME_TRANSIENT_PATH and
DB_RUNTIME_PERSISTENT_PATH value references in the thunderid release template to
read from the nested database.runtime_transient.sqlite.path and
database.runtime_persistent.sqlite.path values, while preserving their existing
defaults and quoting.
---
Nitpick comments:
In `@backend/dbscripts/runtime_persistent/postgres-cleanup.sql`:
- Around line 73-76: Update the REVOKED_TOKEN cleanup DELETE subquery to lock
selected expired rows with FOR UPDATE SKIP LOCKED, ensuring overlapping cleanup
runs process separate batches without blocking or exiting prematurely. Preserve
the existing expiry filter and p_batch_size limit.
In `@backend/dbscripts/runtime_persistent/postgres.sql`:
- Around line 92-102: Standardize the CONSENT table timestamp columns
VALIDITY_TIME, CREATED_AT, and UPDATED_AT on TIMESTAMPTZ with CURRENT_TIMESTAMP
defaults, replacing the inconsistent timestamp default usage while preserving
the existing nullable and column constraints.
In `@backend/dbscripts/runtime_transient/postgres.sql`:
- Around line 90-113: Add a DEFAULT child partition for RUNTIME_STORE after the
explicit namespace partitions, preserving all existing namespace-specific
partitions. Ensure unmatched NAMESPACE values are routed to this fallback
partition instead of failing inserts.
🪄 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.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 69bb2041-3290-4b8b-be05-4bf1e37e4978
📒 Files selected for processing (25)
.github/actions/run-integration-tests/action.ymlbackend/cmd/server/config/default.jsonbackend/cmd/server/deployment.yamlbackend/dbscripts/runtime_persistent/postgres-cleanup.sqlbackend/dbscripts/runtime_persistent/postgres.sqlbackend/dbscripts/runtime_persistent/sqlite.sqlbackend/dbscripts/runtime_transient/postgres-cleanup.sqlbackend/dbscripts/runtime_transient/postgres.sqlbackend/dbscripts/runtime_transient/sqlite.sqlbackend/internal/system/database/provider/dbprovider.gobackend/scripts/cleanup_runtime_transient_db.shbackend/tests/resources/deployment.yamlbuild.ps1build.shinstall/helm/values.yamlinstall/local-development/docker-compose.ymlinstall/openchoreo/helm/charts/thunderid-component/templates/thunderid-component.yamlinstall/openchoreo/helm/charts/thunderid-component/templates/thunderid-release.yamlinstall/openchoreo/helm/charts/thunderid-component/values.yamlinstall/openchoreo/helm/values.yamlinstall/openchoreo/thunderid-oc-resourcetype/templates/thunderid-resourcetype.yamltests/integration/resources/deployment.yamltests/integration/resources/scripts/setup-test-config.ps1tests/integration/resources/scripts/setup-test-config.shtests/integration/testutils/test_utils.go
| "type": "sqlite", | ||
| "sqlite": { | ||
| "path": "database/runtime-transient.db", | ||
| "path": "database/runtime_transient.db", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The SQLite filename rename needs an upgrade migration or explicit guard. Existing deployments still contain the hyphenated files, so these new defaults can cause the application to create fresh databases and hide existing runtime data.
backend/cmd/server/config/default.json#L48-L48: migrate or detectdatabase/runtime-transient.db.backend/cmd/server/config/default.json#L87-L87: migrate or detectdatabase/runtime-persistent.db.backend/cmd/server/deployment.yaml#L27-L27: preserve the transient database during deployment upgrades.backend/cmd/server/deployment.yaml#L45-L45: preserve the persistent database during deployment upgrades.install/helm/values.yaml#L311-L311: document or automate migration for Helm SQLite PVCs.install/helm/values.yaml#L377-L377: document or automate migration for Helm SQLite PVCs.install/openchoreo/helm/charts/thunderid-component/templates/thunderid-component.yaml#L81-L81: preserve the transient database during workload upgrades.install/openchoreo/helm/charts/thunderid-component/templates/thunderid-component.yaml#L85-L85: preserve the persistent database during workload upgrades.install/openchoreo/helm/charts/thunderid-component/values.yaml#L52-L52: document or automate migration for component SQLite storage.install/openchoreo/helm/charts/thunderid-component/values.yaml#L58-L58: document or automate migration for component SQLite storage.install/openchoreo/helm/values.yaml#L70-L70: document or automate migration for OpenChoreo SQLite storage.install/openchoreo/helm/values.yaml#L76-L76: document or automate migration for OpenChoreo SQLite storage.install/openchoreo/thunderid-oc-resourcetype/templates/thunderid-resourcetype.yaml#L418-L418: preserve the transient database in generated deployments.install/openchoreo/thunderid-oc-resourcetype/templates/thunderid-resourcetype.yaml#L434-L434: preserve the persistent database in generated deployments.
Based on PR objectives, existing SQLite deployments must rename their files or update configured paths.
📍 Affects 7 files
backend/cmd/server/config/default.json#L48-L48(this comment)backend/cmd/server/config/default.json#L87-L87backend/cmd/server/deployment.yaml#L27-L27backend/cmd/server/deployment.yaml#L45-L45install/helm/values.yaml#L311-L311install/helm/values.yaml#L377-L377install/openchoreo/helm/charts/thunderid-component/templates/thunderid-component.yaml#L81-L81install/openchoreo/helm/charts/thunderid-component/templates/thunderid-component.yaml#L85-L85install/openchoreo/helm/charts/thunderid-component/values.yaml#L52-L52install/openchoreo/helm/charts/thunderid-component/values.yaml#L58-L58install/openchoreo/helm/values.yaml#L70-L70install/openchoreo/helm/values.yaml#L76-L76install/openchoreo/thunderid-oc-resourcetype/templates/thunderid-resourcetype.yaml#L418-L418install/openchoreo/thunderid-oc-resourcetype/templates/thunderid-resourcetype.yaml#L434-L434
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/cmd/server/config/default.json` at line 48, The SQLite filename
rename must preserve existing databases by migrating or detecting the hyphenated
filenames and updating configured paths consistently. Apply this to
backend/cmd/server/config/default.json lines 48-48 and 87-87; preserve both
databases in backend/cmd/server/deployment.yaml lines 27-27 and 45-45; document
or automate PVC migration in install/helm/values.yaml lines 311-311 and 377-377;
preserve storage in
install/openchoreo/helm/charts/thunderid-component/templates/thunderid-component.yaml
lines 81-81 and 85-85; document or automate migration in its values.yaml lines
52-52 and 58-58 and install/openchoreo/helm/values.yaml lines 70-70 and 76-76;
and preserve both databases in
install/openchoreo/thunderid-oc-resourcetype/templates/thunderid-resourcetype.yaml
lines 418-418 and 434-434.
| dbNameRuntimeTransient = "runtime_transient" | ||
| dbNameEntity = "entity" | ||
| dbNameRuntimePersistent = "runtime-persistent" | ||
| dbNameRuntimePersistent = "runtime_persistent" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.
Missing documentation:
- SQLite deployment migrations: update deployment guides to indicate that existing SQLite deployments must rename their
runtime-transient.dbandruntime-persistent.dbfiles toruntime_transient.dbandruntime_persistent.db, respectively, and update any configured paths in their deployment settings.
(As per path instructions: "Check whether this change introduces or modifies configuration options... post a single consolidated PR-level comment".)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/system/database/provider/dbprovider.go` around lines 42 -
44, Update the relevant SQLite deployment documentation to describe the
migration from runtime-transient.db and runtime-persistent.db to
runtime_transient.db and runtime_persistent.db, including updating configured
deployment paths to match the renamed files.
Source: Path instructions
| value: {{ .Values.database.runtime_transient.path | default "database/runtime_transient.db" | quote }} | ||
| - key: DB_ENTITY_PATH | ||
| value: {{ .Values.database.entity.path | default "database/entitydb.db" | quote }} | ||
| - key: DB_RUNTIME_PERSISTENT_PATH | ||
| value: {{ .Values.database.runtime_persistent.path | default "database/runtime-persistent.db" | quote }} | ||
| value: {{ .Values.database.runtime_persistent.path | default "database/runtime_persistent.db" | quote }} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Read runtime SQLite paths from the nested values structure.
install/openchoreo/helm/charts/thunderid-component/values.yaml defines database.runtime_transient.sqlite.path and database.runtime_persistent.sqlite.path, but this template reads database.runtime_transient.path and database.runtime_persistent.path. Custom paths are therefore ignored and the defaults are always selected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@install/openchoreo/helm/charts/thunderid-component/templates/thunderid-release.yaml`
around lines 76 - 80, Update the DB_RUNTIME_TRANSIENT_PATH and
DB_RUNTIME_PERSISTENT_PATH value references in the thunderid release template to
read from the nested database.runtime_transient.sqlite.path and
database.runtime_persistent.sqlite.path values, while preserving their existing
defaults and quoting.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Superseded by #4164, which combines the code and documentation changes into a single PR targeting the |
2 participants
Purpose
The
backend/dbscripts/schema directories used kebab-case names (runtime-transient,runtime-persistent) while the Postgres database names and the config keys use underscores (runtime_transient,runtime_persistent).Because the two forms differed, the Helm quickstart schema-apply loop, which reuses the database name as the directory path, resolved
dbscripts/runtime_transient/which does not exist. The failure was silent:caterrored to stderr whilepsqlexited 0 on empty input, so the two runtime databases were left with zero tables and no error was reported.This aligns the physical artifact names to underscores so the directory name equals the database name and the config key for all four databases, removing the mismatch at its source.
🔧 Summary of Breaking Changes
The two runtime
dbscripts/directories and their SQLite files are renamed:dbscripts/runtime-transient/todbscripts/runtime_transient/dbscripts/runtime-persistent/todbscripts/runtime_persistent/runtime-transient.db/runtime-persistent.dbtoruntime_transient.db/runtime_persistent.dbPostgres database names and config keys are unchanged (already underscore).
💥 Impact
Existing SQLite deployments that use the default paths
database/runtime-transient.dbanddatabase/runtime-persistent.dbwill, after this change, look for the underscore file names and create fresh empty databases if the old files are not renamed. Anyone applying schemas from an olddbscripts/checkout by the kebab directory name must use the underscore names.🔄 Migration Guide
For SQLite, rename the existing data files to
runtime_transient.dbandruntime_persistent.db, or point the configured paths at the old files. Postgres deployments need no change; the database names were alreadyruntime_transientandruntime_persistent.Documentation updates for the pages that still reference the kebab paths will
follow in a separate docs PR.
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit