chore: migrate deployment variable values to db#817
Merged
adityachoudhari26 merged 2 commits intomainfrom Feb 26, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds persistent storage and API for deployment variable values: new DB table and migration, sqlc queries and models, repository interfaces and DB/in-memory implementations, store wiring with a DB option and legacy migration, and test updates switching selectors to CEL format. Changes
Sequence Diagram(s)sequenceDiagram
participant Store as Workspace Store
participant Legacy as InMemory Repo
participant NewRepo as DB-backed Repo
participant DB as Postgres
Store->>Legacy: Items() (legacy values)
Store->>Store: setStatus("Migrating legacy deployment variable values")
loop per value
Store->>NewRepo: Set(value) -> Upsert
NewRepo->>DB: UpsertDeploymentVariableValue
DB-->>NewRepo: OK
NewRepo-->>Store: OK / error
end
Store->>Store: migration complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
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 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/db/src/schema/deployment-variable.ts (1)
26-34: Add an index fordeployment_variable_idto protect read-path performance.
ListDeploymentVariableValuesByVariableIDwill scan bydeployment_variable_id; indexing this column will avoid full scans as data grows.💡 Suggested schema update
import { bigint, + index, jsonb, pgTable, text, unique, uuid, } from "drizzle-orm/pg-core"; -export const deploymentVariableValue = pgTable("deployment_variable_value", { - id: uuid("id").primaryKey().defaultRandom(), - deploymentVariableId: uuid("deployment_variable_id") - .notNull() - .references(() => deploymentVariable.id, { onDelete: "cascade" }), - value: jsonb("value").notNull(), - resourceSelector: text("resource_selector"), - priority: bigint("priority", { mode: "number" }).notNull().default(0), -}); +export const deploymentVariableValue = pgTable( + "deployment_variable_value", + { + id: uuid("id").primaryKey().defaultRandom(), + deploymentVariableId: uuid("deployment_variable_id") + .notNull() + .references(() => deploymentVariable.id, { onDelete: "cascade" }), + value: jsonb("value").notNull(), + resourceSelector: text("resource_selector"), + priority: bigint("priority", { mode: "number" }).notNull().default(0), + }, + (t) => [index("deployment_variable_value_deployment_variable_id_idx").on(t.deploymentVariableId)], +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/schema/deployment-variable.ts` around lines 26 - 34, The schema is missing an index on deploymentVariableValue.deploymentVariableId which will cause ListDeploymentVariableValuesByVariableID to perform full scans as data grows; add an index on the deploymentVariableId column (either by declaring an index in the table schema or by creating a migration that creates an index, e.g., CREATE INDEX ix_deployment_variable_value_deployment_variable_id ON deployment_variable_value (deployment_variable_id)) so reads by deployment_variable_id use the index and avoid full table scans.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/workspace-engine/pkg/db/queries/deployment_variables.sql`:
- Around line 33-36: The query for ListDeploymentVariableValuesByVariableID in
deployment_variable_value should be made deterministic by adding an explicit
ORDER BY clause; update the SELECT in ListDeploymentVariableValuesByVariableID
to append "ORDER BY priority DESC, id" so values with higher priority come first
and ties are consistently ordered by id.
In `@apps/workspace-engine/pkg/workspace/store/deployment_variables.go`:
- Around line 56-59: The Values method is silently swallowing repository errors
by returning an empty map when
d.store.DeploymentVariableValues.repo.GetByVariableID(variableId) fails; change
Values to propagate the error instead of hiding it: update the Values method
signature to return (map[string]*oapi.DeploymentVariableValue, error) if it does
not already, and on err return nil and the wrapped error (or the original error)
so callers can handle/log it; update all callers of Values to handle the
returned error accordingly.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/mapper.go`:
- Around line 101-107: The mapping code suppresses errors from
e.ResourceSelector.AsCelSelector(), causing invalid selectors to be dropped
silently; change the logic in mapper.go to mirror other field validations (like
ID/DeploymentVariableID/Value) by checking the error returned from
AsCelSelector() and returning/propagating it (or wrapping it) instead of
ignoring it, and only set resourceSelector = pgtype.Text{String: cel.Cel, Valid:
true} when err == nil and cel.Cel != "" so invalid selector parsing surfaces to
the caller (repo.go:125) for proper handling.
In `@packages/db/drizzle/0152_true_mystique.sql`:
- Around line 1-9: Add a non-unique index on the foreign-key column
"deployment_variable_id" in the deployment_variable_value table to support
efficient cascade deletes and lookups: update the migration that creates
"deployment_variable_value" to create an index on "deployment_variable_id"
(referencing the table name deployment_variable_value and column
deployment_variable_id and the FK constraint
deployment_variable_value_deployment_variable_id_deployment_variable_id_fk) and
also add the same index definition to the schema file
packages/db/src/schema/deployment-variable.ts so the index exists both in
migrations and in the TypeScript schema.
---
Nitpick comments:
In `@packages/db/src/schema/deployment-variable.ts`:
- Around line 26-34: The schema is missing an index on
deploymentVariableValue.deploymentVariableId which will cause
ListDeploymentVariableValuesByVariableID to perform full scans as data grows;
add an index on the deploymentVariableId column (either by declaring an index in
the table schema or by creating a migration that creates an index, e.g., CREATE
INDEX ix_deployment_variable_value_deployment_variable_id ON
deployment_variable_value (deployment_variable_id)) so reads by
deployment_variable_id use the index and avoid full table scans.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
apps/workspace-engine/pkg/db/deployment_variables.sql.goapps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/queries/deployment_variables.sqlapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/db/sqlc.yamlapps/workspace-engine/pkg/workspace/store/deployment_variable_values.goapps/workspace-engine/pkg/workspace/store/deployment_variables.goapps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/mapper.goapps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/repo.goapps/workspace-engine/pkg/workspace/store/repository/db/repo.goapps/workspace-engine/pkg/workspace/store/repository/interfaces.goapps/workspace-engine/pkg/workspace/store/repository/memory/repo.goapps/workspace-engine/pkg/workspace/store/repository/repo.goapps/workspace-engine/pkg/workspace/store/store.goapps/workspace-engine/svc/workspaceconsumer/consumer.goapps/workspace-engine/test/e2e/engine_deployment_variable_test.goapps/workspace-engine/test/integration/dbtest.goapps/workspace-engine/test/integration/opts.gopackages/db/drizzle/0152_true_mystique.sqlpackages/db/drizzle/meta/0152_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/deployment-variable.ts
💤 Files with no reviewable changes (1)
- apps/workspace-engine/test/integration/opts.go
Comment on lines
+56
to
+59
| dvvs, err := d.store.DeploymentVariableValues.repo.GetByVariableID(variableId) | ||
| if err != nil { | ||
| return make(map[string]*oapi.DeploymentVariableValue) | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t silently swallow repository errors in Values.
Returning an empty map on failure masks data-access issues and can lead to incorrect variable resolution behavior without any signal.
🔧 Suggested fix
func (d *DeploymentVariables) Values(variableId string) map[string]*oapi.DeploymentVariableValue {
dvvs, err := d.store.DeploymentVariableValues.repo.GetByVariableID(variableId)
if err != nil {
+ log.Error("Failed to list deployment variable values by variable ID", "variableId", variableId, "error", err)
return make(map[string]*oapi.DeploymentVariableValue)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/store/deployment_variables.go` around
lines 56 - 59, The Values method is silently swallowing repository errors by
returning an empty map when
d.store.DeploymentVariableValues.repo.GetByVariableID(variableId) fails; change
Values to propagate the error instead of hiding it: update the Values method
signature to return (map[string]*oapi.DeploymentVariableValue, error) if it does
not already, and on err return nil and the wrapped error (or the original error)
so callers can handle/log it; update all callers of Values to handle the
returned error accordingly.
Comment on lines
+101
to
+107
| var resourceSelector pgtype.Text | ||
| if e.ResourceSelector != nil { | ||
| cel, err := e.ResourceSelector.AsCelSelector() | ||
| if err == nil && cel.Cel != "" { | ||
| resourceSelector = pgtype.Text{String: cel.Cel, Valid: true} | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify selector union behavior and mapper error handling paths.
# 1) Inspect Selector union helpers in generated OAPI to confirm AsCelSelector failure modes.
echo "=== OAPI Selector union and AsCelSelector ==="
rg -n -C3 'type Selector|AsCelSelector\(|UnmarshalJSON\(' apps/workspace-engine/pkg/oapi/oapi.gen.go | head -100
# 2) Inspect current mapper handling around AsCelSelector in this file.
echo -e "\n=== Mapper AsCelSelector handling ==="
rg -n -C4 'ToValueUpsertParams|AsCelSelector|resourceSelector' apps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/mapper.go
# 3) Check where deployment variable values are built/validated before persistence.
echo -e "\n=== Deployment variable validation and building ==="
rg -n -C3 'DeploymentVariableValue|ResourceSelector' apps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/mapper.go | head -80Repository: ctrlplanedev/ctrlplane
Length of output: 5765
🏁 Script executed:
#!/bin/bash
# Check callers of ToValueUpsertParams to understand usage context
rg -n 'ToValueUpsertParams' apps/workspace-engine/pkg --type go -A 3 -B 1Repository: ctrlplanedev/ctrlplane
Length of output: 1382
Handle AsCelSelector() errors explicitly to maintain consistency with other field validations.
On lines 103–107, AsCelSelector() errors are silently ignored while ID, DeploymentVariableID, and Value errors are explicitly propagated (lines 93, 98, 111). This inconsistency allows invalid selectors to persist as NULL without error, potentially dropping resource constraints unexpectedly. Since the function signature already supports error returns and the caller explicitly handles errors (repo.go:125), validation failures should be surfaced rather than masked.
Proposed fix
var resourceSelector pgtype.Text
if e.ResourceSelector != nil {
cel, err := e.ResourceSelector.AsCelSelector()
- if err == nil && cel.Cel != "" {
- resourceSelector = pgtype.Text{String: cel.Cel, Valid: true}
- }
+ if err != nil {
+ return db.UpsertDeploymentVariableValueParams{}, fmt.Errorf("parse resource_selector: %w", err)
+ }
+ if cel.Cel != "" {
+ resourceSelector = pgtype.Text{String: cel.Cel, Valid: true}
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/mapper.go`
around lines 101 - 107, The mapping code suppresses errors from
e.ResourceSelector.AsCelSelector(), causing invalid selectors to be dropped
silently; change the logic in mapper.go to mirror other field validations (like
ID/DeploymentVariableID/Value) by checking the error returned from
AsCelSelector() and returning/propagating it (or wrapping it) instead of
ignoring it, and only set resourceSelector = pgtype.Text{String: cel.Cel, Valid:
true} when err == nil and cel.Cel != "" so invalid selector parsing surfaces to
the caller (repo.go:125) for proper handling.
Comment on lines
+1
to
+9
| CREATE TABLE "deployment_variable_value" ( | ||
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, | ||
| "deployment_variable_id" uuid NOT NULL, | ||
| "value" jsonb NOT NULL, | ||
| "resource_selector" text, | ||
| "priority" bigint DEFAULT 0 NOT NULL | ||
| ); | ||
| --> statement-breakpoint | ||
| ALTER TABLE "deployment_variable_value" ADD CONSTRAINT "deployment_variable_value_deployment_variable_id_deployment_variable_id_fk" FOREIGN KEY ("deployment_variable_id") REFERENCES "public"."deployment_variable"("id") ON DELETE cascade ON UPDATE no action; No newline at end of file |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the migration file and check its contents
fd '0152_true_mystique.sql' -type fRepository: ctrlplanedev/ctrlplane
Length of output: 236
🏁 Script executed:
# Search for any indexes related to deployment_variable_value in migrations
rg 'CREATE INDEX.*deployment_variable' --type sql -A 2 -B 2Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check if schema.sql or other schema files have an index for deployment_variable_id
rg 'deployment_variable_id.*idx|idx.*deployment_variable_id' --type sqlRepository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Look for all migration files in the db directory to see the sequence
fd '\.sql$' packages/db/drizzle | sortRepository: ctrlplanedev/ctrlplane
Length of output: 7212
🏁 Script executed:
# Read the migration file
cat packages/db/drizzle/0152_true_mystique.sqlRepository: ctrlplanedev/ctrlplane
Length of output: 584
🏁 Script executed:
# Search for any CREATE INDEX statements in all migration files
rg 'CREATE INDEX.*deployment_variable' packages/db/drizzle/ -A 3Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Check if there's a schema.sql file that mirrors the migrations
fd 'schema\.sql' packages/db/Repository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
# Search for deployment_variable_id in migration files to see all references
rg 'deployment_variable_id' packages/db/drizzle/ | head -30Repository: ctrlplanedev/ctrlplane
Length of output: 3858
🏁 Script executed:
# Check if other foreign keys have indexes created after them
rg 'ADD CONSTRAINT.*FOREIGN KEY' packages/db/drizzle/ -A 5 | head -50Repository: ctrlplanedev/ctrlplane
Length of output: 9178
🏁 Script executed:
# Look for indexes on other FK columns to see if there's a pattern
rg 'CREATE INDEX' packages/db/drizzle/ | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 3432
🏁 Script executed:
# Check the Drizzle schema definition file for deployment_variable_value
find packages/db -name '*.ts' -type f | xargs rg 'deployment_variable_value|deploymentVariableValue' | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 189
🏁 Script executed:
# Read the full schema definition for deploymentVariableValue
cat packages/db/src/schema/deployment-variable.tsRepository: ctrlplanedev/ctrlplane
Length of output: 1044
Add an index for deployment_variable_id.
FK columns are not auto-indexed in PostgreSQL. This index is necessary for efficient cascade deletes when parent deployment_variable records are removed, and for lookups filtering by this column.
🔧 Suggested fix
CREATE TABLE "deployment_variable_value" (
"id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL,
"deployment_variable_id" uuid NOT NULL,
"value" jsonb NOT NULL,
"resource_selector" text,
"priority" bigint DEFAULT 0 NOT NULL
);
--> statement-breakpoint
ALTER TABLE "deployment_variable_value" ADD CONSTRAINT "deployment_variable_value_deployment_variable_id_deployment_variable_id_fk" FOREIGN KEY ("deployment_variable_id") REFERENCES "public"."deployment_variable"("id") ON DELETE cascade ON UPDATE no action;
+--> statement-breakpoint
+CREATE INDEX "deployment_variable_value_deployment_variable_id_idx" ON "deployment_variable_value" ("deployment_variable_id");Also add the index to packages/db/src/schema/deployment-variable.ts:
export const deploymentVariableValue = pgTable("deployment_variable_value", {
id: uuid("id").primaryKey().defaultRandom(),
deploymentVariableId: uuid("deployment_variable_id")
.notNull()
.references(() => deploymentVariable.id, { onDelete: "cascade" }),
value: jsonb("value").notNull(),
resourceSelector: text("resource_selector"),
priority: bigint("priority", { mode: "number" }).notNull().default(0),
+}, (t) => [index("deployment_variable_value_deployment_variable_id_idx").on(t.deploymentVariableId)]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/drizzle/0152_true_mystique.sql` around lines 1 - 9, Add a
non-unique index on the foreign-key column "deployment_variable_id" in the
deployment_variable_value table to support efficient cascade deletes and
lookups: update the migration that creates "deployment_variable_value" to create
an index on "deployment_variable_id" (referencing the table name
deployment_variable_value and column deployment_variable_id and the FK
constraint
deployment_variable_value_deployment_variable_id_deployment_variable_id_fk) and
also add the same index definition to the schema file
packages/db/src/schema/deployment-variable.ts so the index exists both in
migrations and in the TypeScript schema.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/db/queries/deployment_variables.sql (1)
50-55: Consider adding deterministic ordering for workspace-scoped value listing.This query can return rows in nondeterministic order, which can cause unstable behavior/tests if consumers iterate directly.
♻️ Suggested query update
-- name: ListDeploymentVariableValuesByWorkspaceID :many SELECT dvv.id, dvv.deployment_variable_id, dvv.value, dvv.resource_selector, dvv.priority FROM deployment_variable_value dvv INNER JOIN deployment_variable dv ON dv.id = dvv.deployment_variable_id INNER JOIN deployment d ON d.id = dv.deployment_id -WHERE d.workspace_id = $1; +WHERE d.workspace_id = $1 +ORDER BY dvv.deployment_variable_id, dvv.priority DESC, dvv.id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/db/queries/deployment_variables.sql` around lines 50 - 55, The ListDeploymentVariableValuesByWorkspaceID query returns rows in nondeterministic order; make the result deterministic by adding an ORDER BY clause (e.g., order by dvv.priority DESC, dvv.id ASC) to the SELECT that references deployment_variable_value (aliases dvv) so consumers/tests iterate predictably; update the SQL for the query named ListDeploymentVariableValuesByWorkspaceID accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/workspace-engine/pkg/db/queries/deployment_variables.sql`:
- Around line 50-55: The ListDeploymentVariableValuesByWorkspaceID query returns
rows in nondeterministic order; make the result deterministic by adding an ORDER
BY clause (e.g., order by dvv.priority DESC, dvv.id ASC) to the SELECT that
references deployment_variable_value (aliases dvv) so consumers/tests iterate
predictably; update the SQL for the query named
ListDeploymentVariableValuesByWorkspaceID accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/workspace-engine/pkg/db/deployment_variables.sql.goapps/workspace-engine/pkg/db/queries/deployment_variables.sql
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 by CodeRabbit
New Features
Refactor