refactor: convert to dependency to live on version not deployment#1084
Merged
adityachoudhari26 merged 3 commits intomainfrom Apr 29, 2026
Merged
refactor: convert to dependency to live on version not deployment#1084adityachoudhari26 merged 3 commits intomainfrom
adityachoudhari26 merged 3 commits intomainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR migrates dependency management from deployment-level to deployment-version-level scope. It removes deployment-scoped dependency schemas and endpoints across the OpenAPI spec, database, and route handlers, replacing them with version-scoped equivalents. The database table shifts from Changes
Sequence Diagram(s)No sequence diagrams are applicable to this change, as it is primarily a scope migration and refactoring that preserves existing control flows rather than introducing new interactions between components. Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 36 minutes and 14 seconds.Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors “deployment dependency” edges to be pinned to a specific deployment_version rather than the parent deployment, updating storage, API surface, and the workspace-engine evaluator to reflect per-version semantics.
Changes:
- Replaces
deployment_dependencywithdeployment_version_dependency(schema + migration) and updates sqlc queries accordingly. - Removes deployment-level dependency API endpoints and adds deployment-version dependency endpoints (OpenAPI + Express handlers).
- Updates workspace-engine dependency evaluator/getters and adds end-to-end controller tests for per-version dependency behavior.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/db/src/schema/deployment.ts | Removes the deployment_dependency table definition from Drizzle schema. |
| packages/db/src/schema/deployment-version.ts | Adds deployment_version_dependency table definition (PK: version + dependency deployment). |
| packages/db/drizzle/meta/_journal.json | Registers the new Drizzle migration tag. |
| packages/db/drizzle/0191_equal_talisman.sql | Creates deployment_version_dependency and drops deployment_dependency. |
| apps/workspace-engine/test/controllers/harness/mocks.go | Updates test harness getter mock to return dependencies keyed by deployment version ID. |
| apps/workspace-engine/test/controllers/deployment_version_dependency_test.go | Adds controller-level tests covering satisfied/unsatisfied/multi-edge/per-version pinning behavior. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentversiondependency/getter_postgres.go | Changes dependency lookup to use deployment version ID and new sqlc query. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentversiondependency/getter.go | Updates getter interface contract/docs to be per-version. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentversiondependency/deployment_version_dependency_test.go | Adjusts evaluator unit test scope to include Version. |
| apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentversiondependency/deployment_version_dependency.go | Evaluator now scopes on Version+Resource and queries dependencies by version ID. |
| apps/workspace-engine/pkg/oapi/oapi.gen.go | Removes the no-longer-used DeploymentDependency model from generated types. |
| apps/workspace-engine/pkg/db/queries/deployments.sql | Renames dependency query to GetDeploymentDependenciesByVersionID; adds downstream lookup query. |
| apps/workspace-engine/pkg/db/deployments.sql.go | Regenerates sqlc code for updated/new queries. |
| apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet | Removes DeploymentDependency schema from workspace-engine OpenAPI spec. |
| apps/workspace-engine/oapi/openapi.json | Removes DeploymentDependency from generated OpenAPI JSON. |
| apps/api/src/types/openapi.ts | Updates generated API typings: remove deployment dependency endpoints; add deployment-version dependency endpoints + schemas. |
| apps/api/src/routes/v1/workspaces/deployments.ts | Removes Express handlers/routes for deployment-level dependency CRUD. |
| apps/api/src/routes/v1/workspaces/deployment-versions.ts | Adds Express handlers/routes for deployment-version dependency list/upsert/delete and workspace scoping helper. |
| apps/api/openapi/schemas/deployments.jsonnet | Renames request/response schemas to UpsertDeploymentVersionDependencyRequest + DeploymentVersionDependency. |
| apps/api/openapi/paths/deploymentversions.jsonnet | Adds OpenAPI paths for listing/upserting/deleting deployment-version dependencies. |
| apps/api/openapi/paths/deployments.jsonnet | Removes OpenAPI paths for deployment-level dependencies. |
| apps/api/openapi/openapi.json | Updates generated OpenAPI JSON to reflect the new endpoints and schemas. |
Comments suppressed due to low confidence (1)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentversiondependency/deployment_version_dependency.go:57
- The evaluator now operates on deployment versions (scope uses
ScopeVersionandscope.Version.Id), but the user-facing messages still say "Deployment dependency". This makes evaluation output harder to interpret when debugging. Consider updating these strings to explicitly say "Deployment version dependency" (and similarly for the error message prefix) to match the new rule type/semantics.
edges, err := e.getters.GetDependencies(ctx, scope.Version.Id)
if err != nil {
span.RecordError(err)
return results.NewDeniedResult(
fmt.Sprintf("Deployment dependency: failed to load dependencies: %v", err),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+137
to
+163
| const listDeploymentVersionDependencies: AsyncTypedHandler< | ||
| "/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}/dependencies", | ||
| "get" | ||
| > = async (req, res) => { | ||
| const { workspaceId, deploymentVersionId } = req.params; | ||
|
|
||
| const found = await loadVersionInWorkspace(workspaceId, deploymentVersionId); | ||
| if (found == null) throw new ApiError("Deployment version not found", 404); | ||
|
|
||
| const rows = await db | ||
| .select() | ||
| .from(schema.deploymentVersionDependency) | ||
| .where( | ||
| eq( | ||
| schema.deploymentVersionDependency.deploymentVersionId, | ||
| deploymentVersionId, | ||
| ), | ||
| ) | ||
| .orderBy(asc(schema.deploymentVersionDependency.dependencyDeploymentId)); | ||
|
|
||
| res.status(200).json(rows); | ||
| }; | ||
|
|
||
| const upsertDeploymentVersionDependency: AsyncTypedHandler< | ||
| "/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}/dependencies/{dependencyDeploymentId}", | ||
| "put" | ||
| > = async (req, res) => { |
| "version_selector" text DEFAULT 'false' NOT NULL, | ||
| CONSTRAINT "deployment_version_dependency_deployment_version_id_dependency_deployment_id_pk" PRIMARY KEY("deployment_version_id","dependency_deployment_id") | ||
| ); | ||
| --> statement-breakpoint |
| "version_selector" text DEFAULT 'false' NOT NULL, | ||
| CONSTRAINT "deployment_version_dependency_deployment_version_id_dependency_deployment_id_pk" PRIMARY KEY("deployment_version_id","dependency_deployment_id") | ||
| ); | ||
| --> statement-breakpoint |
| versionSelector: text("version_selector").notNull().default("false"), | ||
| }, | ||
| (t) => [ | ||
| primaryKey({ columns: [t.deploymentVersionId, t.dependencyDeploymentId] }), |
![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: 5
🧹 Nitpick comments (3)
apps/workspace-engine/pkg/db/deployments.sql.go (2)
81-82: Drop the redundant comment in generated query accessor.This note mostly repeats what the query/function name already conveys and adds little extra signal.
As per coding guidelines, "Do not add comments that simply restate what the code does."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/db/deployments.sql.go` around lines 81 - 82, Remove the redundant explanatory comment above the generated query accessor in apps/workspace-engine/pkg/db/deployments.sql.go (the line "Dependencies are pinned per deployment_version, so this returns the dep edges that belong to a single deployment_version row."); since the function/query name already conveys this, delete that comment so the generated accessor remains concise and free of restating-comments.
127-132: Make downstream deployment ID results deterministic.
SELECT DISTINCTwithoutORDER BYcan return rows in varying order, which can cause unstable downstream iteration/test behavior.Proposed diff
const getDeploymentsWithVersionsDependingOn = `-- name: GetDeploymentsWithVersionsDependingOn :many SELECT DISTINCT dv.deployment_id FROM deployment_version_dependency dvd JOIN deployment_version dv ON dv.id = dvd.deployment_version_id WHERE dvd.dependency_deployment_id = $1 +ORDER BY dv.deployment_id `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/db/deployments.sql.go` around lines 127 - 132, The query in the GetDeploymentsWithVersionsDependingOn SQL constant uses SELECT DISTINCT without ORDER BY, causing non-deterministic row ordering; make results deterministic by adding an ORDER BY clause (e.g., ORDER BY dv.deployment_id) to the SQL in GetDeploymentsWithVersionsDependingOn so callers receive a stable, predictable sequence of deployment IDs.apps/api/openapi/openapi.json (1)
5320-5320: Pick one deployment-version path style before adding more endpoints.These new routes use
/deploymentversions/..., but the spec already exposes/deployment-versions/{deploymentVersionId}/user-approval-recordsfor the same resource family. Shipping both spellings in v1 will make the API harder to navigate and produces awkward generated client surfaces.Also applies to: 5372-5372
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/openapi/openapi.json` at line 5320, The spec is exposing two inconsistent path styles for the same resource ("/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}/dependencies" vs "/v1/.../deployment-versions/{deploymentVersionId}/user-approval-records"); pick one canonical path style (either "deployment-versions" with hyphen or "deploymentversions" without) and update all new endpoints to match it. Rename the new path(s) and their operationId/route names to use the chosen style, update any schema $ref/parameter references that point to the old path, and adjust related tests, client generators, and docs to use the single consistent path (ensure you also change the duplicate occurrence around lines referenced by the reviewer).
🤖 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/api/openapi/openapi.json`:
- Around line 5320-5514: You changed the dependency endpoints from
/v1/workspaces/{workspaceId}/deployments/{deploymentId}/dependencies... to
/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}/dependencies...,
which is a breaking v1 API change; restore a deprecated compatibility shim by
reintroducing the original routes (the /deployments/{deploymentId}/dependencies
paths) and have them delegate to the new handlers (reuse the logic behind
operationId listDeploymentVersionDependencies,
requestDeploymentVersionDependencyDeletion, and
requestDeploymentVersionDependencyUpsert) while marking those old paths
deprecated in the OpenAPI spec and adding a comment/response header indicating
deprecation and migration instructions for one release cycle.
In `@apps/api/src/routes/v1/workspaces/deployment-versions.ts`:
- Around line 210-214: The async calls to enqueueReleaseTargetsForDeployment are
currently fire-and-forget and can cause unhandled rejections; update each call
site that currently calls enqueueReleaseTargetsForDeployment(db, workspaceId,
found.deployment.id) (and the second similar call later in the file) to
explicitly handle the returned promise using the non-blocking pattern: void
enqueueReleaseTargetsForDeployment(...).catch(err =>
logger.warn("enqueueReleaseTargetsForDeployment failed", { err, workspaceId,
deploymentId: found.deployment.id })); use the existing logger variable in this
module so the call remains non-blocking but any rejection is logged.
In `@packages/db/drizzle/0191_equal_talisman.sql`:
- Around line 1-6: The join table deployment_version_dependency lacks workspace
scoping: add a NOT NULL workspace_id uuid column, include workspace_id in the
primary key (so PK becomes (workspace_id, deployment_version_id,
dependency_deployment_id)), and add foreign key constraints that pair
workspace_id with each id (e.g., (workspace_id, deployment_version_id) ->
deployment_version and (workspace_id, dependency_deployment_id) ->
deployment/deployment_version as appropriate) to enforce tenant isolation;
update constraint names to reflect the new PK/FK columns.
- Line 8: The migration currently drops the legacy table via DROP TABLE
"deployment_dependency" which will lose all existing dependency rows; instead,
implement a data migration that preserves existing data by selecting from
deployment_dependency and inserting into deployment_version_dependency (mapping
columns and foreign keys as needed), validate and backfill any new
columns/defaults, and only drop "deployment_dependency" after successful
copy/verification (or keep it as a fallback). Update the migration that contains
DROP TABLE "deployment_dependency" to perform: 1) create any missing
columns/indexes on deployment_version_dependency, 2) insert transformed rows
from deployment_dependency into deployment_version_dependency, 3) verify
counts/constraints, and finally 4) drop the legacy table (or mark it deprecated)
— reference the table names deployment_dependency and
deployment_version_dependency and the DROP TABLE statement to locate where to
apply these changes.
In `@packages/db/src/schema/deployment-version.ts`:
- Around line 69-83: The table definition deploymentVersionDependency is missing
workspaceId which allows cross-tenant edges; add a workspaceId column (uuid
referencing workspace.id with notNull and onDelete cascade), include workspaceId
in the primaryKey alongside deploymentVersionId and dependencyDeploymentId, and
update the migration/schema to create that column and constraint; also update
any insert/update code paths that write to deploymentVersionDependency (and any
relational helpers) to set and validate workspaceId equals the workspaceId of
deploymentVersionId and dependencyDeploymentId so the same-workspace invariant
is enforced at write time.
---
Nitpick comments:
In `@apps/api/openapi/openapi.json`:
- Line 5320: The spec is exposing two inconsistent path styles for the same
resource
("/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}/dependencies"
vs "/v1/.../deployment-versions/{deploymentVersionId}/user-approval-records");
pick one canonical path style (either "deployment-versions" with hyphen or
"deploymentversions" without) and update all new endpoints to match it. Rename
the new path(s) and their operationId/route names to use the chosen style,
update any schema $ref/parameter references that point to the old path, and
adjust related tests, client generators, and docs to use the single consistent
path (ensure you also change the duplicate occurrence around lines referenced by
the reviewer).
In `@apps/workspace-engine/pkg/db/deployments.sql.go`:
- Around line 81-82: Remove the redundant explanatory comment above the
generated query accessor in apps/workspace-engine/pkg/db/deployments.sql.go (the
line "Dependencies are pinned per deployment_version, so this returns the dep
edges that belong to a single deployment_version row."); since the
function/query name already conveys this, delete that comment so the generated
accessor remains concise and free of restating-comments.
- Around line 127-132: The query in the GetDeploymentsWithVersionsDependingOn
SQL constant uses SELECT DISTINCT without ORDER BY, causing non-deterministic
row ordering; make results deterministic by adding an ORDER BY clause (e.g.,
ORDER BY dv.deployment_id) to the SQL in GetDeploymentsWithVersionsDependingOn
so callers receive a stable, predictable sequence of deployment IDs.
🪄 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
Run ID: d018bf80-8ad1-431b-b531-0124aa571e7b
📒 Files selected for processing (23)
apps/api/openapi/openapi.jsonapps/api/openapi/paths/deployments.jsonnetapps/api/openapi/paths/deploymentversions.jsonnetapps/api/openapi/schemas/deployments.jsonnetapps/api/src/routes/v1/workspaces/deployment-versions.tsapps/api/src/routes/v1/workspaces/deployments.tsapps/api/src/types/openapi.tsapps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/schemas/deployments.jsonnetapps/workspace-engine/pkg/db/deployments.sql.goapps/workspace-engine/pkg/db/queries/deployments.sqlapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentversiondependency/deployment_version_dependency.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentversiondependency/deployment_version_dependency_test.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentversiondependency/getter.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/deploymentversiondependency/getter_postgres.goapps/workspace-engine/test/controllers/deployment_version_dependency_test.goapps/workspace-engine/test/controllers/harness/mocks.gopackages/db/drizzle/0191_equal_talisman.sqlpackages/db/drizzle/meta/0191_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/deployment-version.tspackages/db/src/schema/deployment.ts
💤 Files with no reviewable changes (5)
- apps/workspace-engine/oapi/openapi.json
- apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet
- packages/db/src/schema/deployment.ts
- apps/workspace-engine/pkg/oapi/oapi.gen.go
- apps/api/openapi/paths/deployments.jsonnet
Comment on lines
+5320
to
+5514
| "/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}/dependencies": { | ||
| "get": { | ||
| "description": "Returns the dependency edges declared by this deployment version.", | ||
| "operationId": "listDeploymentVersionDependencies", | ||
| "parameters": [ | ||
| { | ||
| "description": "ID of the workspace", | ||
| "in": "path", | ||
| "name": "workspaceId", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| { | ||
| "description": "ID of the deployment version", | ||
| "in": "path", | ||
| "name": "deploymentVersionId", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "items": { | ||
| "$ref": "#/components/schemas/DeploymentVersionDependency" | ||
| }, | ||
| "type": "array" | ||
| } | ||
| } | ||
| }, | ||
| "description": "OK response" | ||
| }, | ||
| "404": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ErrorResponse" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Resource not found" | ||
| } | ||
| }, | ||
| "summary": "List deployment-version dependencies" | ||
| } | ||
| }, | ||
| "/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}/dependencies/{dependencyDeploymentId}": { | ||
| "delete": { | ||
| "operationId": "requestDeploymentVersionDependencyDeletion", | ||
| "parameters": [ | ||
| { | ||
| "description": "ID of the workspace", | ||
| "in": "path", | ||
| "name": "workspaceId", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| { | ||
| "description": "ID of the deployment version", | ||
| "in": "path", | ||
| "name": "deploymentVersionId", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| { | ||
| "description": "ID of the dependency deployment", | ||
| "in": "path", | ||
| "name": "dependencyDeploymentId", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "202": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/DeploymentRequestAccepted" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Accepted response" | ||
| }, | ||
| "400": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ErrorResponse" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Invalid request" | ||
| }, | ||
| "404": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ErrorResponse" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Resource not found" | ||
| } | ||
| }, | ||
| "summary": "Delete deployment-version dependency" | ||
| }, | ||
| "put": { | ||
| "description": "Declare or update a version-selector dependency from this deployment version to another deployment. Identified by the (deploymentVersionId, dependencyDeploymentId) pair.", | ||
| "operationId": "requestDeploymentVersionDependencyUpsert", | ||
| "parameters": [ | ||
| { | ||
| "description": "ID of the workspace", | ||
| "in": "path", | ||
| "name": "workspaceId", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| { | ||
| "description": "ID of the deployment version", | ||
| "in": "path", | ||
| "name": "deploymentVersionId", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| { | ||
| "description": "ID of the dependency deployment", | ||
| "in": "path", | ||
| "name": "dependencyDeploymentId", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| ], | ||
| "requestBody": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/UpsertDeploymentVersionDependencyRequest" | ||
| } | ||
| } | ||
| }, | ||
| "required": true | ||
| }, | ||
| "responses": { | ||
| "202": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/DeploymentRequestAccepted" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Accepted response" | ||
| }, | ||
| "400": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ErrorResponse" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Invalid request" | ||
| }, | ||
| "404": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ErrorResponse" | ||
| } | ||
| } | ||
| }, | ||
| "description": "Resource not found" | ||
| } | ||
| }, | ||
| "summary": "Upsert deployment-version dependency" | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
This is a breaking v1 API replacement unless the old routes stay alive.
Switching dependency management from /deployments/{deploymentId}/dependencies... to /deploymentversions/{deploymentVersionId}/dependencies... changes both the URL shape and the required identifier. Existing clients that only know a deploymentId cannot migrate transparently and will start failing once the old handlers disappear. Please keep a deprecated compatibility shim for at least one release, or treat this as an explicit versioned API break with migration notes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/openapi/openapi.json` around lines 5320 - 5514, You changed the
dependency endpoints from
/v1/workspaces/{workspaceId}/deployments/{deploymentId}/dependencies... to
/v1/workspaces/{workspaceId}/deploymentversions/{deploymentVersionId}/dependencies...,
which is a breaking v1 API change; restore a deprecated compatibility shim by
reintroducing the original routes (the /deployments/{deploymentId}/dependencies
paths) and have them delegate to the new handlers (reuse the logic behind
operationId listDeploymentVersionDependencies,
requestDeploymentVersionDependencyDeletion, and
requestDeploymentVersionDependencyUpsert) while marking those old paths
deprecated in the OpenAPI spec and adding a comment/response header indicating
deprecation and migration instructions for one release cycle.
Comment on lines
+210
to
+214
| enqueueReleaseTargetsForDeployment( | ||
| db, | ||
| workspaceId, | ||
| found.deployment.id, | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 '\benqueueReleaseTargetsForDeployment\s*\(' apps packages -g '*.ts'Repository: ctrlplanedev/ctrlplane
Length of output: 8531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the function definition of enqueueReleaseTargetsForDeployment
rg -n 'export.*enqueueReleaseTargetsForDeployment|function enqueueReleaseTargetsForDeployment' apps packages -g '*.ts' -A 5Repository: ctrlplanedev/ctrlplane
Length of output: 528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the specific file around the flagged lines
cat -n apps/api/src/routes/v1/workspaces/deployment-versions.ts | sed -n '205,260p'Repository: ctrlplanedev/ctrlplane
Length of output: 1979
Handle the enqueue promise explicitly to prevent unhandled rejections.
enqueueReleaseTargetsForDeployment is async and these fire-and-forget calls do not detach or catch rejections. If the promise rejects after the 202 response is sent, it becomes an unhandled rejection. Use the pattern void enqueueReleaseTargetsForDeployment(...).catch(err => logger.warn(...)) to maintain non-blocking behavior while preventing unhandled rejections.
This applies to lines 210–214 and 252–256 in this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/v1/workspaces/deployment-versions.ts` around lines 210 -
214, The async calls to enqueueReleaseTargetsForDeployment are currently
fire-and-forget and can cause unhandled rejections; update each call site that
currently calls enqueueReleaseTargetsForDeployment(db, workspaceId,
found.deployment.id) (and the second similar call later in the file) to
explicitly handle the returned promise using the non-blocking pattern: void
enqueueReleaseTargetsForDeployment(...).catch(err =>
logger.warn("enqueueReleaseTargetsForDeployment failed", { err, workspaceId,
deploymentId: found.deployment.id })); use the existing logger variable in this
module so the call remains non-blocking but any rejection is logged.
Comment on lines
+1
to
+6
| CREATE TABLE "deployment_version_dependency" ( | ||
| "deployment_version_id" uuid NOT NULL, | ||
| "dependency_deployment_id" uuid NOT NULL, | ||
| "version_selector" text DEFAULT 'false' NOT NULL, | ||
| CONSTRAINT "deployment_version_dependency_deployment_version_id_dependency_deployment_id_pk" PRIMARY KEY("deployment_version_id","dependency_deployment_id") | ||
| ); |
Contributor
There was a problem hiding this comment.
Add tenant scoping to this join table.
deployment_version_dependency is missing workspace_id, so the database can accept an edge from a version in workspace A to a dependency deployment in workspace B as long as both UUIDs exist. That weakens multi-tenant isolation at the schema level and forces every caller to infer tenancy through joins. As per coding guidelines: All database tables include workspaceId for multi-tenant isolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/drizzle/0191_equal_talisman.sql` around lines 1 - 6, The join
table deployment_version_dependency lacks workspace scoping: add a NOT NULL
workspace_id uuid column, include workspace_id in the primary key (so PK becomes
(workspace_id, deployment_version_id, dependency_deployment_id)), and add
foreign key constraints that pair workspace_id with each id (e.g.,
(workspace_id, deployment_version_id) -> deployment_version and (workspace_id,
dependency_deployment_id) -> deployment/deployment_version as appropriate) to
enforce tenant isolation; update constraint names to reflect the new PK/FK
columns.
| CONSTRAINT "deployment_version_dependency_deployment_version_id_dependency_deployment_id_pk" PRIMARY KEY("deployment_version_id","dependency_deployment_id") | ||
| ); | ||
| --> statement-breakpoint | ||
| DROP TABLE "deployment_dependency" CASCADE;--> statement-breakpoint |
Contributor
There was a problem hiding this comment.
Don't drop the legacy dependency table without a data migration.
Line 8 deletes every existing deployment_dependency row, and the refactored evaluator now reads only deployment_version_dependency. On upgrade, all previously configured dependencies disappear until users recreate them manually.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/drizzle/0191_equal_talisman.sql` at line 8, The migration
currently drops the legacy table via DROP TABLE "deployment_dependency" which
will lose all existing dependency rows; instead, implement a data migration that
preserves existing data by selecting from deployment_dependency and inserting
into deployment_version_dependency (mapping columns and foreign keys as needed),
validate and backfill any new columns/defaults, and only drop
"deployment_dependency" after successful copy/verification (or keep it as a
fallback). Update the migration that contains DROP TABLE "deployment_dependency"
to perform: 1) create any missing columns/indexes on
deployment_version_dependency, 2) insert transformed rows from
deployment_dependency into deployment_version_dependency, 3) verify
counts/constraints, and finally 4) drop the legacy table (or mark it deprecated)
— reference the table names deployment_dependency and
deployment_version_dependency and the DROP TABLE statement to locate where to
apply these changes.
Comment on lines
+69
to
+83
| export const deploymentVersionDependency = pgTable( | ||
| "deployment_version_dependency", | ||
| { | ||
| deploymentVersionId: uuid("deployment_version_id") | ||
| .references(() => deploymentVersion.id, { onDelete: "cascade" }) | ||
| .notNull(), | ||
| dependencyDeploymentId: uuid("dependency_deployment_id") | ||
| .references(() => deployment.id, { onDelete: "cascade" }) | ||
| .notNull(), | ||
| versionSelector: text("version_selector").notNull().default("false"), | ||
| }, | ||
| (t) => [ | ||
| primaryKey({ columns: [t.deploymentVersionId, t.dependencyDeploymentId] }), | ||
| ], | ||
| ); |
Contributor
There was a problem hiding this comment.
This schema still needs workspaceId.
Without tenant scoping on deploymentVersionDependency, Drizzle can persist cross-workspace dependency edges because deploymentVersionId and dependencyDeploymentId are each valid on their own. Please carry workspaceId on the row and enforce the same invariant in the migration/write path. As per coding guidelines: All database tables include workspaceId for multi-tenant isolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/schema/deployment-version.ts` around lines 69 - 83, The table
definition deploymentVersionDependency is missing workspaceId which allows
cross-tenant edges; add a workspaceId column (uuid referencing workspace.id with
notNull and onDelete cascade), include workspaceId in the primaryKey alongside
deploymentVersionId and dependencyDeploymentId, and update the migration/schema
to create that column and constraint; also update any insert/update code paths
that write to deploymentVersionDependency (and any relational helpers) to set
and validate workspaceId equals the workspaceId of deploymentVersionId and
dependencyDeploymentId so the same-workspace invariant is enforced at write
time.
Member
Author
Review responseWorked through all 9 inline comments. Summary of what's fixed vs. what we're skipping with rationale. ✅ Fixed
❌ Skipping (with reasons)
Action needed before merge
|
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
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
Tests