chore: convert endpoint to use go engine #690
Conversation
📊 Code Coverage Reportworkspace-engine coverage: |
WalkthroughThe PR migrates deployment version jobs listing from database-driven aggregation to Workspace Engine API calls, refactors OpenAPI schemas to support additional properties in JsonSelector and rename job links to metadata, and updates the SDK to introduce environment progression rules with mandatory deployment and environment fields. Changes
Sequence DiagramsequenceDiagram
actor Client
participant API as API Router
participant WSE as Workspace Engine API
participant Transform as Data Transform Layer
participant Schema as Internal Schema
Client->>API: GET /deployment-versions/.../jobs-list
activate API
API->>API: Extract workspace/path IDs
API->>WSE: getDeploymentVersionJobsList(workspaceId)
activate WSE
WSE-->>API: OapiResponse (jobs, deployments, environments, resources, metadata)
deactivate WSE
API->>Transform: mapOapiToSchema(response)
activate Transform
Transform->>Transform: convertOapiEnvironment()
Transform->>Transform: convertOapiDeployment()
Transform->>Transform: convertOapiResource()
Transform->>Transform: getJobLinks(metadata)
Transform->>Schema: Build DeploymentVersionJobsListResponse
Transform-->>API: Transformed data
deactivate Transform
API-->>Client: DeploymentVersionJobsListResponse
deactivate API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The changes span multiple layers (OpenAPI schemas, SDK types, API routing, and data transformation), involve substantial refactoring from database-driven logic to external API integration, introduce several new transformation functions with error handling, and require careful verification of data shape conversions and field mapping consistency. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
📊 DB Package Test Coveragepkg/db coverage: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
apps/workspace-engine/oapi/spec/paths/deployment-version.jsonnet (1)
48-57: metadata shape OK; consider UUID formats and strict objects.
- Add "format: uuid" to id/resourceId/environmentId/deploymentId.
- Consider "additionalProperties: false" on the job object to catch stray fields.
apps/workspace-engine/oapi/openapi.json (1)
1218-1254: jobs-list: metadata switch and required fields look consistent.
- Nice: metadata as string map, made required.
- Optional: add "format: uuid" to ids; consider "additionalProperties: false" on job objects.
packages/api/src/router/deployment-version-jobs-list.ts (5)
28-34: links vs metadata: align response naming.Engine jobs expose metadata; this route maps it to links. Confirm consumers expect links (not metadata) or rename for consistency.
52-58: Add a runtime guard for JsonSelector.Casting unknown JSON to ResourceCondition is risky. Validate shape before assigning.
-const convertOapiSelectorToResourceCondition = ( +const isResourceCondition = (v: unknown): v is ResourceCondition => { + // TODO: tighten per ResourceCondition variants + return typeof v === "object" && v !== null; +}; +const convertOapiSelectorToResourceCondition = ( selector?: WorkspaceEngine["schemas"]["Selector"], ): ResourceCondition | null => { if (selector == null) return null; - if ("json" in selector) return selector.json as ResourceCondition; + if ("json" in selector && isResourceCondition(selector.json)) { + return selector.json; + } return null; };
75-86: Confirm Environment.directory default.Setting directory: "" — ensure SCHEMA.Environment expects a non-null string; if nullable, prefer null to empty string.
111-136: Exhaustiveness guard for status mapping.Future enum additions will cause runtime fallthrough. Add an assertNever to fail fast at compile-time.
const convertOapiJobStatusToSchema = ( status: WorkspaceEngine["schemas"]["JobStatus"], ): SCHEMA.JobStatus => { switch (status) { @@ case "externalRunNotFound": return "external_run_not_found"; } + // Ensure exhaustive + const _exhaustive: never = status; + return _exhaustive; };
140-144: Unused input: search.Input includes search but it’s not used. Either implement filtering or remove from the contract.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/workspace-engine/oapi/openapi.json(4 hunks)apps/workspace-engine/oapi/spec/paths/deployment-version.jsonnet(2 hunks)apps/workspace-engine/oapi/spec/schemas/core.jsonnet(1 hunks)packages/api/src/router/deployment-version-jobs-list.ts(2 hunks)packages/workspace-engine-sdk/src/schema.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/workspace-engine/oapi/openapi.jsonpackages/workspace-engine-sdk/src/schema.tspackages/api/src/router/deployment-version-jobs-list.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
packages/workspace-engine-sdk/src/schema.tspackages/api/src/router/deployment-version-jobs-list.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
packages/workspace-engine-sdk/src/schema.tspackages/api/src/router/deployment-version-jobs-list.ts
🧬 Code graph analysis (1)
packages/api/src/router/deployment-version-jobs-list.ts (6)
packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/validators/src/resources/conditions/resource-condition.ts (1)
ResourceCondition(29-39)apps/webservice/src/app/api/v1/jobs/[jobId]/get-job.ts (1)
getJobLinks(41-57)packages/logger/src/index.ts (1)
logger(66-66)packages/api/src/workspace-engine-client.ts (1)
getWorkspaceEngineClient(9-12)packages/db/src/schema/release.ts (1)
releaseTarget(21-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: workspace-engine-tests
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (6)
apps/workspace-engine/oapi/spec/schemas/core.jsonnet (1)
9-9: Broadened JsonSelector is fine; ensure consumers validate.Allowing arbitrary keys is OK. Please confirm downstream code that casts selector.json to typed conditions performs runtime validation (see convertOapiSelectorToResourceCondition usage).
apps/workspace-engine/oapi/spec/paths/deployment-version.jsonnet (1)
32-32: Required fields expansion looks good.The ReleaseTarget block now includes ids and related entities; this aligns with the new engine response.
packages/workspace-engine-sdk/src/schema.ts (2)
481-484: JsonSelector as Record<string, unknown> is correct.Works with the spec broadening. Ensure consumers validate before use.
732-749: Jobs-list typing matches spec (metadata required).Type changes align with path/schema updates. Make sure all callers are regenerated against this SDK.
apps/workspace-engine/oapi/openapi.json (1)
485-487: JsonSelector additionalProperties enabled — OK.Matches core.jsonnet; TS SDK now reflects unknown map.
packages/api/src/router/deployment-version-jobs-list.ts (1)
6-6: No issues found with eq and takeFirst imports.Both symbols are correctly re-exported by @ctrlplane/db:
eqviaexport * from "drizzle-orm/sql"andtakeFirstviaexport * from "./common.js". The import statement is valid.
| const getWorkspaceId = async (tx: Tx, versionId: string) => | ||
| tx | ||
| .select() | ||
| .from(SCHEMA.deploymentVersion) | ||
| .where(eq(SCHEMA.deploymentVersion.id, versionId)) | ||
| .then(takeFirst); | ||
|
|
||
| const getVersionSubquery = (db: Tx, versionId: string) => | ||
| db | ||
| .select({ | ||
| jobId: SCHEMA.job.id, | ||
| jobCreatedAt: SCHEMA.job.createdAt, | ||
| jobStatus: SCHEMA.job.status, | ||
| jobExternalId: SCHEMA.job.externalId, | ||
| jobMetadataKey: SCHEMA.jobMetadata.key, | ||
| jobMetadataValue: SCHEMA.jobMetadata.value, | ||
| releaseTargetId: SCHEMA.versionRelease.releaseTargetId, | ||
| }) | ||
| .from(SCHEMA.versionRelease) | ||
| .innerJoin( | ||
| SCHEMA.release, | ||
| eq(SCHEMA.release.versionReleaseId, SCHEMA.versionRelease.id), | ||
| ) | ||
| .innerJoin( | ||
| SCHEMA.releaseJob, | ||
| eq(SCHEMA.releaseJob.releaseId, SCHEMA.release.id), | ||
| ) | ||
| .innerJoin(SCHEMA.job, eq(SCHEMA.releaseJob.jobId, SCHEMA.job.id)) | ||
| .leftJoin(SCHEMA.jobMetadata, eq(SCHEMA.jobMetadata.jobId, SCHEMA.job.id)) | ||
| .where( | ||
| and( | ||
| eq(SCHEMA.versionRelease.versionId, versionId), | ||
| or( | ||
| eq(SCHEMA.jobMetadata.key, ReservedMetadataKey.Links), | ||
| isNull(SCHEMA.jobMetadata.key), | ||
| ), | ||
| ), | ||
| ) | ||
| .as("version_subquery"); | ||
|
|
||
| const getReleaseTargets = (db: Tx, version: SCHEMA.DeploymentVersion) => { | ||
| const versionSubquery = getVersionSubquery(db, version.id); | ||
|
|
||
| return db | ||
| .select() | ||
| .from(SCHEMA.releaseTarget) | ||
| .innerJoin( | ||
| SCHEMA.environment, | ||
| eq(SCHEMA.releaseTarget.environmentId, SCHEMA.environment.id), | ||
| ) | ||
| .innerJoin( | ||
| SCHEMA.deployment, | ||
| eq(SCHEMA.releaseTarget.deploymentId, SCHEMA.deployment.id), | ||
| ) | ||
| .innerJoin( | ||
| SCHEMA.resource, | ||
| eq(SCHEMA.resource.id, SCHEMA.releaseTarget.resourceId), | ||
| eq(SCHEMA.deploymentVersion.deploymentId, SCHEMA.deployment.id), | ||
| ) | ||
| .leftJoin( | ||
| versionSubquery, | ||
| eq(versionSubquery.releaseTargetId, SCHEMA.releaseTarget.id), | ||
| ) | ||
| .where(and(eq(SCHEMA.releaseTarget.deploymentId, version.deploymentId))); | ||
| .innerJoin(SCHEMA.system, eq(SCHEMA.deployment.systemId, SCHEMA.system.id)) | ||
| .where(eq(SCHEMA.deploymentVersion.id, versionId)) | ||
| .then(takeFirst) | ||
| .then(({ system }) => system.workspaceId); |
There was a problem hiding this comment.
Surface 404 on unknown versionId.
takeFirst throws; currently this will bubble as a 500. Translate to a typed 404 to clients.
+import { TRPCError } from "@trpc/server";
@@
-const getWorkspaceId = async (tx: Tx, versionId: string) =>
- tx
+const getWorkspaceId = async (tx: Tx, versionId: string) =>
+ tx
.select()
@@
- .then(takeFirst)
- .then(({ system }) => system.workspaceId);
+ .then(takeFirst)
+ .then(({ system }) => system.workspaceId)
+ .catch(() => {
+ throw new TRPCError({ code: "NOT_FOUND", message: "Deployment version not found" });
+ });🤖 Prompt for AI Agents
In packages/api/src/router/deployment-version-jobs-list.ts around lines 15–26,
the helper getWorkspaceId uses takeFirst which throws on no result and currently
bubbles as a 500; change the implementation to explicitly detect a missing row
and throw a typed 404 error instead. After executing the query, check the
returned row (do not rely on takeFirst throwing) and if it's undefined/null
throw the project's HTTP/NotFound error type (e.g., createHttpError(404, ...) or
the app's NotFound/TRPC error) with a clear message like "deployment version not
found", otherwise return the workspaceId from the row. Ensure the function
preserves existing typing/signatures.
| const getJobLinks = (metadata: Record<string, string>) => { | ||
| const linksStr = metadata[ReservedMetadataKey.Links] ?? "{}"; | ||
|
|
||
| try { | ||
| const links = JSON.parse(linksStr) as Record<string, string>; | ||
| return links; | ||
| } catch (error) { | ||
| logger.error("Error parsing job links", { | ||
| error, | ||
| metadata, | ||
| }); | ||
| return {}; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Avoid logging full metadata payload on parse errors.
Metadata may contain sensitive info. Log only the invalid links string, and redact/truncate.
- try {
- const links = JSON.parse(linksStr) as Record<string, string>;
+ try {
+ const links = JSON.parse(linksStr) as Record<string, string>;
return links;
} catch (error) {
- logger.error("Error parsing job links", {
- error,
- metadata,
- });
+ logger.error("Error parsing job links", {
+ error,
+ linksSample: linksStr.slice(0, 500),
+ });
return {};
}🤖 Prompt for AI Agents
In packages/api/src/router/deployment-version-jobs-list.ts around lines 60 to
73, the catch block currently logs the entire metadata object on JSON parse
errors which may contain sensitive data; change the error logging to omit
metadata and instead log only the raw links string (linksStr) after applying
redaction/truncation (e.g., replace with a fixed-length prefix + "…(truncated)"
if longer than N chars, or redact obvious secrets) and include the parse error;
ensure logger.error receives a small safe payload: the error and the sanitized
linksStr only.
| .query<DeploymentVersionJobsListResponse>( | ||
| async ({ ctx, input: { versionId } }) => { | ||
| const workspaceId = await getWorkspaceId(ctx.db, versionId); | ||
| const client = getWorkspaceEngineClient(); | ||
| const resp = await client.GET( | ||
| "/v1/workspaces/{workspaceId}/deployment-versions/{versionId}/jobs-list", | ||
| { | ||
| params: { | ||
| path: { | ||
| workspaceId, | ||
| versionId, | ||
| }, | ||
| }, | ||
| }, | ||
| ); | ||
| return (resp.data ?? []).map((env) => ({ | ||
| environment: convertOapiEnvironmentToSchema(env.environment), | ||
| releaseTargets: env.releaseTargets.map((releaseTarget) => ({ | ||
| deployment: convertOapiDeploymentToSchema(releaseTarget.deployment), | ||
| environment: convertOapiEnvironmentToSchema( | ||
| releaseTarget.environment, | ||
| ), | ||
| resource: convertOapiResourceToSchema(releaseTarget.resource), | ||
| id: releaseTarget.id, | ||
| resourceId: releaseTarget.resourceId, | ||
| environmentId: releaseTarget.environmentId, | ||
| deploymentId: releaseTarget.deploymentId, | ||
| desiredReleaseId: null, | ||
| desiredVersionId: null, | ||
| jobs: (releaseTarget.jobs ?? []).map((job) => ({ | ||
| createdAt: new Date(job.createdAt), | ||
| externalId: job.externalId ?? null, | ||
| id: job.id, | ||
| status: convertOapiJobStatusToSchema(job.status), | ||
| links: getJobLinks(job.metadata ?? {}), | ||
| })), | ||
| })), | ||
| })); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Handle non-2xx from engine; don't silently return [].
Currently resp.data ?? [] hides errors (e.g., 404/5xx). Map to typed TRPC errors.
- .query<DeploymentVersionJobsListResponse>(
- async ({ ctx, input: { versionId } }) => {
+ .query<DeploymentVersionJobsListResponse>(async ({ ctx, input: { versionId } }) => {
const workspaceId = await getWorkspaceId(ctx.db, versionId);
const client = getWorkspaceEngineClient();
- const resp = await client.GET(
+ const resp = await client.GET(
"/v1/workspaces/{workspaceId}/deployment-versions/{versionId}/jobs-list",
{
params: {
path: {
workspaceId,
versionId,
},
},
},
);
- return (resp.data ?? []).map((env) => ({
+ if (resp.error) {
+ throw new TRPCError({
+ code: resp.response?.status === 404 ? "NOT_FOUND" : "BAD_GATEWAY",
+ message: resp.error.message ?? "Workspace engine request failed",
+ });
+ }
+ return (resp.data ?? []).map((env) => ({
environment: convertOapiEnvironmentToSchema(env.environment),
@@
}));
},
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .query<DeploymentVersionJobsListResponse>( | |
| async ({ ctx, input: { versionId } }) => { | |
| const workspaceId = await getWorkspaceId(ctx.db, versionId); | |
| const client = getWorkspaceEngineClient(); | |
| const resp = await client.GET( | |
| "/v1/workspaces/{workspaceId}/deployment-versions/{versionId}/jobs-list", | |
| { | |
| params: { | |
| path: { | |
| workspaceId, | |
| versionId, | |
| }, | |
| }, | |
| }, | |
| ); | |
| return (resp.data ?? []).map((env) => ({ | |
| environment: convertOapiEnvironmentToSchema(env.environment), | |
| releaseTargets: env.releaseTargets.map((releaseTarget) => ({ | |
| deployment: convertOapiDeploymentToSchema(releaseTarget.deployment), | |
| environment: convertOapiEnvironmentToSchema( | |
| releaseTarget.environment, | |
| ), | |
| resource: convertOapiResourceToSchema(releaseTarget.resource), | |
| id: releaseTarget.id, | |
| resourceId: releaseTarget.resourceId, | |
| environmentId: releaseTarget.environmentId, | |
| deploymentId: releaseTarget.deploymentId, | |
| desiredReleaseId: null, | |
| desiredVersionId: null, | |
| jobs: (releaseTarget.jobs ?? []).map((job) => ({ | |
| createdAt: new Date(job.createdAt), | |
| externalId: job.externalId ?? null, | |
| id: job.id, | |
| status: convertOapiJobStatusToSchema(job.status), | |
| links: getJobLinks(job.metadata ?? {}), | |
| })), | |
| })), | |
| })); | |
| }, | |
| ); | |
| .query<DeploymentVersionJobsListResponse>(async ({ ctx, input: { versionId } }) => { | |
| const workspaceId = await getWorkspaceId(ctx.db, versionId); | |
| const client = getWorkspaceEngineClient(); | |
| const resp = await client.GET( | |
| "/v1/workspaces/{workspaceId}/deployment-versions/{versionId}/jobs-list", | |
| { | |
| params: { | |
| path: { | |
| workspaceId, | |
| versionId, | |
| }, | |
| }, | |
| }, | |
| ); | |
| if (resp.error) { | |
| throw new TRPCError({ | |
| code: resp.response?.status === 404 ? "NOT_FOUND" : "BAD_GATEWAY", | |
| message: resp.error.message ?? "Workspace engine request failed", | |
| }); | |
| } | |
| return (resp.data ?? []).map((env) => ({ | |
| environment: convertOapiEnvironmentToSchema(env.environment), | |
| releaseTargets: env.releaseTargets.map((releaseTarget) => ({ | |
| deployment: convertOapiDeploymentToSchema(releaseTarget.deployment), | |
| environment: convertOapiEnvironmentToSchema( | |
| releaseTarget.environment, | |
| ), | |
| resource: convertOapiResourceToSchema(releaseTarget.resource), | |
| id: releaseTarget.id, | |
| resourceId: releaseTarget.resourceId, | |
| environmentId: releaseTarget.environmentId, | |
| deploymentId: releaseTarget.deploymentId, | |
| desiredReleaseId: null, | |
| desiredVersionId: null, | |
| jobs: (releaseTarget.jobs ?? []).map((job) => ({ | |
| createdAt: new Date(job.createdAt), | |
| externalId: job.externalId ?? null, | |
| id: job.id, | |
| status: convertOapiJobStatusToSchema(job.status), | |
| links: getJobLinks(job.metadata ?? {}), | |
| })), | |
| })), | |
| })); | |
| }); |
🤖 Prompt for AI Agents
In packages/api/src/router/deployment-version-jobs-list.ts around lines 152 to
191, the code currently uses resp.data ?? [] which hides non-2xx responses from
the engine; wrap the client.GET call in a try/catch (or inspect resp.status) and
translate non-2xx responses into typed tRPC errors instead of returning an empty
array: import TRPCError from '@trpc/server', if the response status is 404 throw
new TRPCError({ code: 'NOT_FOUND', message: `Engine returned 404 for version
${versionId}` }), for other non-2xx statuses throw new TRPCError({ code:
'INTERNAL_SERVER_ERROR', message: `Engine error ${resp.status}:
${JSON.stringify(resp.data)}` }) (or rethrow caught errors as TRPCError with the
engine response details), only map resp.data when the call succeeded (2xx).
| EnvironmentProgressionRule: { | ||
| dependsOnEnvironmentSelector: components["schemas"]["Selector"]; | ||
| id: string; | ||
| /** | ||
| * Format: int32 | ||
| * @description Maximum age of dependency deployment before blocking progression (prevents stale promotions) | ||
| */ | ||
| maximumAgeHours?: number; | ||
| /** | ||
| * Format: int32 | ||
| * @description Minimum time to wait after the depends on environment is in a success state before the current environment can be deployed | ||
| * @default 0 | ||
| */ | ||
| minimumSockTimeMinutes: number; | ||
| /** | ||
| * Format: float | ||
| * @default 100 | ||
| */ | ||
| minimumSuccessPercentage: number; | ||
| policyId: string; | ||
| successStatuses?: components["schemas"]["JobStatus"][]; | ||
| }; |
There was a problem hiding this comment.
Typo: minimumSockTimeMinutes → minimumSoakTimeMinutes.
Field name reads like a typo (“sock” vs “soak”). Please correct in the source OpenAPI/proto and re‑generate; shipping this would bake a confusing, breaking API.
🤖 Prompt for AI Agents
In packages/workspace-engine-sdk/src/schema.ts around lines 384 to 405, the
field name minimumSockTimeMinutes is a typo and should be
minimumSoakTimeMinutes; update the OpenAPI/proto source to rename the property
to minimumSoakTimeMinutes (preserve the Format: int32, @default 0 and
description text), regenerate the SDK/schema artifacts, and update any
references/usages/tests in the repo to use the new property name so the
generated types and docs match the corrected field.
adityachoudhari26
deleted the
convert-endpoint-deployment-version-jobs
branch
Summary by CodeRabbit