feat: deployment version target selector field to scope version to specific targets#907
feat: deployment version target selector field to scope version to specific targets#907adityachoudhari26 wants to merge 4 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant DB
participant CEL as "CEL runtime"
participant Versions as "Candidate Versions"
Reconciler->>DB: fetch candidate versions (include selector)
DB-->>Reconciler: versions list (selector nullable)
Reconciler->>CEL: compile/eval selector for each non-nil
CEL-->>Reconciler: boolean result / error
Reconciler->>Versions: retain versions where selector==nil OR eval==true (errors -> fail-open)
Reconciler->>Reconciler: continue policy evaluation with filtered versions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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: 1
🤖 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_versions.sql`:
- Around line 9-10: The generated sqlc artifacts are out of sync with the SQL
change that adds selector to the INSERT; regenerate and commit the updated
artifacts so the query constant and param types include selector: run sqlc
generate to update apps/workspace-engine/pkg/db/deployment_versions.sql.go,
verify the upsertDeploymentVersion SQL constant now has the selector
placeholder, ensure the UpsertDeploymentVersionParams struct includes a Selector
field and that the call sites pass selector in the argument list, then rebuild
and commit the regenerated file.
🪄 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: f819708f-62e9-4aff-8628-271af467f2eb
📒 Files selected for processing (5)
apps/api/openapi/schemas/deploymentversions.jsonnetapps/workspace-engine/oapi/spec/schemas/deployments.jsonnetapps/workspace-engine/pkg/db/queries/deployment_versions.sqlapps/workspace-engine/pkg/db/queries/schema.sqlpackages/db/src/schema/deployment-version.ts
There was a problem hiding this comment.
Pull request overview
Adds a selector field to deployment_version so a version can be scoped to specific release targets (via a CEL expression), and exposes that field through the OpenAPI schemas.
Changes:
- Add
selectorcolumn to thedeployment_versionDB schema (Drizzle + workspace-engine SQL schema). - Extend the workspace-engine upsert query to persist
selector. - Add
selectorto OpenAPIDeploymentVersionschemas (workspace-engine + api).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/db/src/schema/deployment-version.ts | Adds selector column to Drizzle table definition for deployment versions. |
| apps/workspace-engine/pkg/db/queries/schema.sql | Adds selector column to workspace-engine’s Postgres schema for sqlc. |
| apps/workspace-engine/pkg/db/queries/deployment_versions.sql | Updates upsert to insert/update selector. |
| apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet | Adds selector to workspace-engine OpenAPI schema for DeploymentVersion. |
| apps/api/openapi/schemas/deploymentversions.jsonnet | Adds selector to API OpenAPI schemas for deployment version request/response models. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| selector: text("selector"), | ||
|
|
||
| workspaceId: uuid("workspace_id").references(() => workspace.id), |
There was a problem hiding this comment.
selector was added to the Drizzle table schema, but there’s no corresponding Drizzle migration in packages/db/drizzle/ to add this column in Postgres. Since packages/db/migrate.ts runs migrations from that folder, deployments will end up with a schema mismatch unless you add an ALTER TABLE deployment_version ADD COLUMN selector text migration (and consider a default if you need non-null semantics).
| -- name: UpsertDeploymentVersion :one | ||
| INSERT INTO deployment_version (id, name, tag, config, job_agent_config, deployment_id, metadata, status, message, workspace_id, created_at) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, COALESCE(sqlc.narg('created_at')::timestamptz, NOW())) | ||
| INSERT INTO deployment_version (id, name, tag, config, job_agent_config, deployment_id, metadata, status, message, selector, workspace_id, created_at) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, COALESCE(sqlc.narg('created_at')::timestamptz, NOW())) | ||
| ON CONFLICT (deployment_id, tag) DO UPDATE | ||
| SET name = EXCLUDED.name, config = EXCLUDED.config, job_agent_config = EXCLUDED.job_agent_config, metadata = EXCLUDED.metadata, status = EXCLUDED.status, message = EXCLUDED.message, workspace_id = EXCLUDED.workspace_id, | ||
| SET name = EXCLUDED.name, config = EXCLUDED.config, job_agent_config = EXCLUDED.job_agent_config, metadata = EXCLUDED.metadata, status = EXCLUDED.status, message = EXCLUDED.message, selector = EXCLUDED.selector, workspace_id = EXCLUDED.workspace_id, | ||
| created_at = CASE WHEN sqlc.narg('created_at')::timestamptz IS NOT NULL THEN EXCLUDED.created_at ELSE deployment_version.created_at END |
There was a problem hiding this comment.
This query change adds a new selector parameter/column, but the committed sqlc outputs are not regenerated: apps/workspace-engine/pkg/db/deployment_versions.sql.go and apps/workspace-engine/pkg/db/models.go currently have no Selector field/param and still generate SQL without the column. Run go generate ./pkg/db (sqlc) and commit the updated generated files so callers can set/read selector.
| status: openapi.schemaRef('DeploymentVersionStatus'), | ||
| message: { type: 'string' }, | ||
| createdAt: { type: 'string', format: 'date-time' }, | ||
| selector: { type: 'string', description: 'CEL expression to scope this version to matching release targets' }, | ||
| }, |
There was a problem hiding this comment.
The spec adds selector to DeploymentVersion, but the generated artifacts are not updated: apps/workspace-engine/oapi/openapi.json and apps/workspace-engine/pkg/oapi/oapi.gen.go currently omit this field. Re-run the oapi generation step and commit the regenerated outputs so the server/client models match the spec.
| name: { type: 'string' }, | ||
| tag: { type: 'string' }, | ||
| createdAt: { type: 'string', format: 'date-time' }, | ||
| selector: { type: 'string', description: 'CEL expression to scope this version to matching release targets' }, | ||
| metadata: { |
There was a problem hiding this comment.
selector was added to the jsonnet schemas, but apps/api/openapi/openapi.json is what the API server imports/uses for request validation and Swagger (apps/api/src/server.ts). That bundled spec currently has no selector on DeploymentVersion, so regenerate/rebuild the OpenAPI JSON output and commit it to keep runtime validation/docs in sync.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.go (1)
368-383: Add a runtime-eval error case for full branch coverage.You cover compile-invalid fail-open, but not eval-time errors (e.g., selector compiles but returns non-boolean). Add one case like
resource.metadata.regionto exercise theEvalBoolerror path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.go` around lines 368 - 383, Add a new test that exercises the eval-time error branch of reconciler.filterByTargetSelector by creating a reconciler with an EvaluatorScope (e.g., evaluator.EvaluatorScope with Resource.Metadata containing a "region" string) and a DeploymentVersion whose Selector is an expression that compiles but evaluates to non-boolean (for example "resource.metadata.region"); call r.filterByTargetSelector() and assert that r.versions remains length 1 (fail-open). Reference the existing TestFilterByTargetSelector_InvalidCEL_FailOpen test and the reconciler.filterByTargetSelector, EvaluatorScope, and DeploymentVersion.Selector symbols to place and implement the new case.
🤖 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/svc/controllers/desiredrelease/reconcile.go`:
- Around line 61-72: The current logic treats selector compile/eval errors as
"fail-open" by appending v to filtered; change this to "fail-closed" so versions
with selector errors are excluded: when celLang.CompileProgram(*v.Selector)
returns err or when celutil.EvalBool(program, celCtx) returns err, do not append
v to filtered, instead log the error (using log.Error or log.Warn with context
including v.Id and the error) and skip to the next version; update the branches
around celLang.CompileProgram, celutil.EvalBool, and the append(filtered, v)
usage to remove the append on errors and ensure only successful EvalBool true
results add v to filtered.
---
Nitpick comments:
In `@apps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.go`:
- Around line 368-383: Add a new test that exercises the eval-time error branch
of reconciler.filterByTargetSelector by creating a reconciler with an
EvaluatorScope (e.g., evaluator.EvaluatorScope with Resource.Metadata containing
a "region" string) and a DeploymentVersion whose Selector is an expression that
compiles but evaluates to non-boolean (for example "resource.metadata.region");
call r.filterByTargetSelector() and assert that r.versions remains length 1
(fail-open). Reference the existing
TestFilterByTargetSelector_InvalidCEL_FailOpen test and the
reconciler.filterByTargetSelector, EvaluatorScope, and
DeploymentVersion.Selector symbols to place and implement the new case.
🪄 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: 0368e75b-c0a4-4bcf-8e65-3ebad213d11b
📒 Files selected for processing (15)
apps/api/openapi/openapi.jsonapps/api/src/types/openapi.tsapps/web/app/api/openapi.tsapps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/pkg/db/deployment_versions.sql.goapps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.goapps/workspace-engine/test/controllers/harness/pipeline_opts.goapps/workspace-engine/test/controllers/version_test.gopackages/db/drizzle/0182_right_bastion.sqlpackages/db/drizzle/meta/0182_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/workspace-engine-sdk/src/schema.ts
✅ Files skipped from review due to trivial changes (2)
- packages/db/drizzle/0182_right_bastion.sql
- packages/db/drizzle/meta/_journal.json
| program, err := celLang.CompileProgram(*v.Selector) | ||
| if err != nil { | ||
| log.Warn("selector compile failed, including version", "version", v.Id, "error", err) | ||
| filtered = append(filtered, v) | ||
| continue | ||
| } | ||
|
|
||
| matches, err := celutil.EvalBool(program, celCtx) | ||
| if err != nil { | ||
| log.Warn("selector eval failed, including version", "version", v.Id, "error", err) | ||
| filtered = append(filtered, v) | ||
| continue |
There was a problem hiding this comment.
Fail-open selector errors can bypass target scoping.
On compile/eval failure, the version is re-added to candidates, so invalid selectors effectively behave like “include”. That can deploy versions outside intended targets.
Suggested change (fail-closed on selector errors)
program, err := celLang.CompileProgram(*v.Selector)
if err != nil {
- log.Warn("selector compile failed, including version", "version", v.Id, "error", err)
- filtered = append(filtered, v)
+ log.Warn("selector compile failed, excluding version", "version", v.Id, "error", err)
continue
}
matches, err := celutil.EvalBool(program, celCtx)
if err != nil {
- log.Warn("selector eval failed, including version", "version", v.Id, "error", err)
- filtered = append(filtered, v)
+ log.Warn("selector eval failed, excluding version", "version", v.Id, "error", err)
continue
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/desiredrelease/reconcile.go` around
lines 61 - 72, The current logic treats selector compile/eval errors as
"fail-open" by appending v to filtered; change this to "fail-closed" so versions
with selector errors are excluded: when celLang.CompileProgram(*v.Selector)
returns err or when celutil.EvalBool(program, celCtx) returns err, do not append
v to filtered, instead log the error (using log.Error or log.Warn with context
including v.Id and the error) and skip to the next version; update the branches
around celLang.CompileProgram, celutil.EvalBool, and the append(filtered, v)
usage to remove the append on errors and ensure only successful EvalBool true
results add v to filtered.
Summary by CodeRabbit
New Features
API Changes
Database Migration
Tests