chore: migrate deployment variables to db#816
Merged
adityachoudhari26 merged 2 commits intomainfrom Feb 26, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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: 2
🧹 Nitpick comments (4)
apps/workspace-engine/pkg/workspace/store/deployments.go (1)
61-70: Consider logging the error before returning an empty map.The error from
GetByDeploymentIDis silently discarded. While returning an empty map is a safe fallback, logging the error would help with debugging if database issues occur.♻️ Proposed fix to add error logging
func (e *Deployments) Variables(deploymentId string) map[string]*oapi.DeploymentVariable { dvs, err := e.store.DeploymentVariables.repo.GetByDeploymentID(deploymentId) if err != nil { + log.Error("Failed to get deployment variables", "deploymentId", deploymentId, "error", err) return make(map[string]*oapi.DeploymentVariable) } vars := make(map[string]*oapi.DeploymentVariable, len(dvs)) for _, dv := range dvs { vars[dv.Key] = dv } return vars }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/store/deployments.go` around lines 61 - 70, The Variables method on Deployments silently swallows errors from e.store.DeploymentVariables.repo.GetByDeploymentID; before returning the empty map, log the error so failures are visible. Update Deployments.Variables to call the component logger (e.logger or e.log if available) or add a logger field to the Deployments struct and use e.logger.Errorf (or appropriate logging method) to record the error with context like the deploymentId and the error value, then continue returning the empty map.apps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/mapper.go (1)
20-25: Silent error handling on JSON unmarshal may hide data issues.If
json.Unmarshalfails, the error is silently swallowed anddefaultValueremainsnil. This could mask database corruption or schema migration issues. Consider logging at debug/warn level when unmarshal fails.🔧 Optional: Log unmarshal failures
var defaultValue *oapi.LiteralValue if row.DefaultValue != nil { lv := &oapi.LiteralValue{} - if err := json.Unmarshal(row.DefaultValue, lv); err == nil { + if err := json.Unmarshal(row.DefaultValue, lv); err != nil { + // Log but don't fail - treat as missing value + log.Debug("failed to unmarshal default_value", "id", row.ID, "error", err) + } else { defaultValue = lv } }🤖 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 20 - 25, The code swallows errors from json.Unmarshal when decoding row.DefaultValue into lv, which can hide DB/schema issues; update the block in mapper.go (the json.Unmarshal on row.DefaultValue that sets defaultValue/lv) to check the err and log a debug or warn with the error and identifying context (e.g., the row's id/name) instead of ignoring it, preserving the existing behavior of leaving defaultValue nil on failure.apps/workspace-engine/pkg/workspace/store/deployment_variables.go (1)
35-41: Unusedidparameter in Upsert.The
idparameter is not used in this method -deploymentVariable.Idis used implicitly throughrepo.Set(). Consider removing it for clarity or documenting why it's kept for interface consistency.🤖 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 35 - 41, The Upsert method on DeploymentVariables has an unused id parameter; either remove the id parameter from the function signature and update any interface and callers that reference DeploymentVariables.Upsert, or make the parameter meaningful by assigning it to the payload before persisting (e.g., set deploymentVariable.Id = id) and then call d.repo.Set(deploymentVariable) and d.store.changeset.RecordUpsert(deploymentVariable); also ensure the public interface/type that declares Upsert is updated consistently.apps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/repo.go (1)
29-32: DB error silently converted to not-found.When
GetDeploymentVariableByIDreturns an error (e.g., connection issue), it's indistinguishable from "not found" since both returnnil, false. This is acceptable for the current interface but could make debugging harder.🔧 Optional: Log DB errors in Get
row, err := db.GetQueries(r.ctx).GetDeploymentVariableByID(r.ctx, uid) if err != nil { + if err != pgx.ErrNoRows { + log.Warn("Failed to get deployment variable", "id", id, "error", err) + } return nil, false }🤖 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/repo.go` around lines 29 - 32, The DB error from db.GetQueries(r.ctx).GetDeploymentVariableByID is being swallowed into a generic not-found return; change the error handling so you distinguish real "not found" from other DB errors: after calling GetDeploymentVariableByID, if errors.Is(err, sql.ErrNoRows) return nil, false, otherwise log the actual err (e.g. using the repository logger like r.logger.Errorf or similar) and then return nil, false to preserve the current API; import errors and database/sql/sql or sql.ErrNoRows as needed.
🤖 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/schema.sql`:
- Around line 236-242: Add a foreign-key constraint on
deployment_variable.deployment_id so rows cannot reference a non-existent
deployment: update the CREATE TABLE for deployment_variable (or run an ALTER
TABLE if schema already exists) to add a FOREIGN KEY referencing the deployment
table's primary key (deployment.id) with an appropriate constraint name (e.g.,
fk_deployment_variable_deployment_id) and choose an on-delete policy such as ON
DELETE CASCADE (to remove variables when a deployment is deleted) or ON DELETE
RESTRICT depending on desired behavior.
In `@packages/db/drizzle/0151_motionless_winter_soldier.sql`:
- Around line 1-9: The migration dropped the unique constraint on
(deployment_id, key) causing duplicate keys per deployment; restore it by adding
a UNIQUE constraint or unique index on the deployment_variable table for the
columns (deployment_id, key) (e.g., ADD CONSTRAINT
deployment_variable_deployment_id_key_unique UNIQUE (deployment_id, key)) so
that the Go code in deployments.go (vars[dv.Key] = dv) and the
UpsertDeploymentVariable logic are protected against duplicate keys; ensure the
constraint name is unique and add it either in the CREATE TABLE statement for
deployment_variable or via an ALTER TABLE immediately after creation.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/workspace/store/deployment_variables.go`:
- Around line 35-41: The Upsert method on DeploymentVariables has an unused id
parameter; either remove the id parameter from the function signature and update
any interface and callers that reference DeploymentVariables.Upsert, or make the
parameter meaningful by assigning it to the payload before persisting (e.g., set
deploymentVariable.Id = id) and then call d.repo.Set(deploymentVariable) and
d.store.changeset.RecordUpsert(deploymentVariable); also ensure the public
interface/type that declares Upsert is updated consistently.
In `@apps/workspace-engine/pkg/workspace/store/deployments.go`:
- Around line 61-70: The Variables method on Deployments silently swallows
errors from e.store.DeploymentVariables.repo.GetByDeploymentID; before returning
the empty map, log the error so failures are visible. Update
Deployments.Variables to call the component logger (e.logger or e.log if
available) or add a logger field to the Deployments struct and use
e.logger.Errorf (or appropriate logging method) to record the error with context
like the deploymentId and the error value, then continue returning the empty
map.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/mapper.go`:
- Around line 20-25: The code swallows errors from json.Unmarshal when decoding
row.DefaultValue into lv, which can hide DB/schema issues; update the block in
mapper.go (the json.Unmarshal on row.DefaultValue that sets defaultValue/lv) to
check the err and log a debug or warn with the error and identifying context
(e.g., the row's id/name) instead of ignoring it, preserving the existing
behavior of leaving defaultValue nil on failure.
In
`@apps/workspace-engine/pkg/workspace/store/repository/db/deploymentvariables/repo.go`:
- Around line 29-32: The DB error from
db.GetQueries(r.ctx).GetDeploymentVariableByID is being swallowed into a generic
not-found return; change the error handling so you distinguish real "not found"
from other DB errors: after calling GetDeploymentVariableByID, if errors.Is(err,
sql.ErrNoRows) return nil, false, otherwise log the actual err (e.g. using the
repository logger like r.logger.Errorf or similar) and then return nil, false to
preserve the current API; import errors and database/sql/sql or sql.ErrNoRows as
needed.
ℹ️ 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 (23)
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/persistence/integration_test.goapps/workspace-engine/pkg/workspace/store/deployment_variables.goapps/workspace-engine/pkg/workspace/store/deployments.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/integration/dbtest.gopackages/db/drizzle/0151_motionless_winter_soldier.sqlpackages/db/drizzle/meta/0151_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/deployment-variable.tspackages/db/src/schema/index.tspackages/db/src/schema/resource-variable.ts
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
Database
deployment_variabletable to persistently store deployment-level variables linked to deployments and workspaces.