chore: jobAgentSelector is required field in deployment schema#940
chore: jobAgentSelector is required field in deployment schema#940adityachoudhari26 merged 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThe PR converts Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 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.
🧹 Nitpick comments (1)
apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go (1)
1172-1183: Consider removing or renaming this duplicate test.With
JobAgentSelectornow astringinstead of*string, there's no distinct "nil" case—only empty string. This test (TestReconcile_NilJobAgentSelector_CreatesFailureJob) is now identical toTestReconcile_NoJobAgentSelector_CreatesFailureJobabove (lines 1159–1170): both setJobAgentSelector = ""and assert the same behavior.Either remove this test as a duplicate, or rename it to test a meaningfully different scenario if one exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go` around lines 1172 - 1183, The test TestReconcile_NilJobAgentSelector_CreatesFailureJob is now a duplicate of TestReconcile_NoJobAgentSelector_CreatesFailureJob because JobAgentSelector is a string (no nil case); remove TestReconcile_NilJobAgentSelector_CreatesFailureJob or rename and change it to cover a distinct scenario (e.g., an invalid non-empty selector value or whitespace-only selector) by updating the test body that manipulates getter.deployment.JobAgentSelector and the corresponding assertions in Reconcile to reflect the new behavior being tested.
🤖 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/svc/controllers/jobeligibility/reconcile_test.go`:
- Around line 1172-1183: The test
TestReconcile_NilJobAgentSelector_CreatesFailureJob is now a duplicate of
TestReconcile_NoJobAgentSelector_CreatesFailureJob because JobAgentSelector is a
string (no nil case); remove TestReconcile_NilJobAgentSelector_CreatesFailureJob
or rename and change it to cover a distinct scenario (e.g., an invalid non-empty
selector value or whitespace-only selector) by updating the test body that
manipulates getter.deployment.JobAgentSelector and the corresponding assertions
in Reconcile to reflect the new behavior being tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59217e09-c544-41f0-a037-f647d47e20f5
📒 Files selected for processing (16)
apps/api/openapi/openapi.jsonapps/api/openapi/schemas/deployments.jsonnetapps/api/src/types/openapi.tsapps/web/app/api/openapi.tsapps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/schemas/deployments.jsonnetapps/workspace-engine/pkg/db/convert.goapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/svc/controllers/deploymentplan/controller.goapps/workspace-engine/svc/controllers/deploymentplan/controller_test.goapps/workspace-engine/svc/controllers/jobdispatch/reconcile.goapps/workspace-engine/svc/controllers/jobdispatch/reconcile_test.goapps/workspace-engine/svc/controllers/jobeligibility/reconcile.goapps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.goapps/workspace-engine/test/controllers/harness/pipeline.gopackages/workspace-engine-sdk/src/schema.ts
There was a problem hiding this comment.
Pull request overview
This PR updates the Deployment OpenAPI schemas and generated clients/server types so jobAgentSelector is treated as a required field (string) rather than an optional/pointer field, and adjusts workspace-engine controllers/tests accordingly.
Changes:
- Make
jobAgentSelectorrequired in Deployment schemas (workspace-engine + API) and propagate through generated OpenAPI artifacts/SDKs. - Update workspace-engine Go code to use
deployment.JobAgentSelectoras astring(remove nil-pointer handling) across controllers and tests. - Regenerate web/API OpenAPI TypeScript types reflecting the schema change (and other spec-driven additions).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/workspace-engine-sdk/src/schema.ts | Generated SDK types: jobAgentSelector now required on Deployment. |
| apps/workspace-engine/test/controllers/harness/pipeline.go | Update test harness Deployment construction to pass selector as string. |
| apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go | Remove nil handling; treat selector as required string and update tracing attrs. |
| apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go | Update tests for non-pointer selector. |
| apps/workspace-engine/svc/controllers/jobdispatch/reconcile.go | Remove nil handling; selector is a string. |
| apps/workspace-engine/svc/controllers/jobdispatch/reconcile_test.go | Update test Deployment construction to pass selector as string. |
| apps/workspace-engine/svc/controllers/deploymentplan/controller.go | Remove nil handling; selector is a string. |
| apps/workspace-engine/svc/controllers/deploymentplan/controller_test.go | Update tests for non-pointer selector. |
| apps/workspace-engine/pkg/oapi/oapi.gen.go | Generated Go types: Deployment.JobAgentSelector becomes string (required JSON field). |
| apps/workspace-engine/pkg/db/convert.go | Always populate JobAgentSelector on OAPI Deployment conversion. |
| apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet | Workspace-engine schema marks jobAgentSelector as required. |
| apps/workspace-engine/oapi/openapi.json | Generated OpenAPI JSON reflects required jobAgentSelector. |
| apps/web/app/api/openapi.ts | Regenerated openapi-typescript output; jobAgentSelector required and other spec updates included. |
| apps/api/src/types/openapi.ts | Generated API types: jobAgentSelector required on Deployment. |
| apps/api/openapi/schemas/deployments.jsonnet | API schema marks jobAgentSelector as required. |
| apps/api/openapi/openapi.json | Generated OpenAPI JSON reflects required jobAgentSelector. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Deployment: { | ||
| type: 'object', | ||
| required: ['id', 'name', 'slug', 'jobAgentConfig'], | ||
| required: ['id', 'name', 'slug', 'jobAgentSelector', 'jobAgentConfig'], |
There was a problem hiding this comment.
Making jobAgentSelector required in the Deployment schema appears inconsistent with current API behavior: apps/api/src/routes/v1/workspaces/deployments.ts formats jobAgentSelector as undefined when the stored value is null or the default string "false", which causes the property to be omitted from JSON responses. Either update the API formatting to always return a string value (e.g. include "false"/""), or keep jobAgentSelector optional in the Deployment response schema to match what the server actually returns.
| required: ['id', 'name', 'slug', 'jobAgentSelector', 'jobAgentConfig'], | |
| required: ['id', 'name', 'slug', 'jobAgentConfig'], |
| func TestReconcile_NilJobAgentSelector_CreatesFailureJob(t *testing.T) { | ||
| rt := testRT() | ||
| release := testRelease(rt) | ||
| getter, setter := setupHappyPath(rt, release) | ||
| getter.deployment.JobAgentSelector = nil | ||
| getter.deployment.JobAgentSelector = "" |
There was a problem hiding this comment.
TestReconcile_NilJobAgentSelector_CreatesFailureJob is now identical to TestReconcile_NoJobAgentSelector_CreatesFailureJob because JobAgentSelector is no longer a pointer and both tests set it to the empty string. Consider removing one of these tests or repurposing it to cover a distinct scenario (e.g. selector explicitly set to "false" or invalid CEL).
| func TestProcess_EmptySelector_CompletesPlan(t *testing.T) { | ||
| dep := testDeployment(false) | ||
| emptySel := "" | ||
| dep.JobAgentSelector = &emptySel | ||
| dep.JobAgentSelector = "" | ||
|
|
There was a problem hiding this comment.
testDeployment(false) already yields an empty JobAgentSelector (zero-value string), so explicitly setting dep.JobAgentSelector = "" here is redundant. This also effectively duplicates the "no selector" behavior already covered by TestProcess_NoAgents_CompletesPlan, and can mask coverage for the distinct case of a non-empty selector with zero matching agents.
Resolves #917
Summary by CodeRabbit
Release Notes
New Features
Breaking Changes
jobAgentSelectorfield is now required for deployment configurations and must be provided in all deployment requests.