feat(cli): attach to existing deployments in the build server #2501
Conversation
🦋 Changeset detectedLatest commit: 3d564cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (22)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
| shortCode: z.string(), | ||
| version: z.string(), | ||
| imageReference: z.string().nullish(), | ||
| imagePlatform: z.string(), |
There was a problem hiding this comment.
this schema is not using strict(), so adding an additional required field won't cause compatibility issues with previous versions of the cli.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/cli-v3/src/commands/deploy.ts (1)
760-765: Add null check for other critical deployment fields.While the code checks for
imageReference, consider also validating other critical fields likeimagePlatformthat are part of theGetDeploymentResponseBodyschema and used later in the build process.const { imageReference, status } = existingDeploymentOrError.data; if (!imageReference) { // this is just an artifact of our current DB schema // `imageReference` is stored as nullable, but it should always exist throw new Error("Existing deployment does not have an image reference"); } + +if (!existingDeploymentOrError.data.imagePlatform) { + throw new Error("Existing deployment does not have an image platform"); +}apps/webapp/app/routes/api.v1.deployments.$deploymentId.ts (1)
72-72: Typed payload via satisfies: nice.Optional: also parameterize json() to propagate the response type to callers.
- } satisfies GetDeploymentResponseBody); + } satisfies GetDeploymentResponseBody) as unknown as ReturnType<typeof json<GetDeploymentResponseBody>>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.ts(3 hunks)packages/cli-v3/src/commands/deploy.ts(4 hunks)packages/core/src/v3/schemas/api.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/core/src/v3/schemas/api.tspackages/cli-v3/src/commands/deploy.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/src/v3/schemas/api.tsapps/webapp/app/routes/api.v1.deployments.$deploymentId.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/routes/api.v1.deployments.$deploymentId.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/routes/api.v1.deployments.$deploymentId.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/routes/api.v1.deployments.$deploymentId.ts
🧠 Learnings (1)
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Import Trigger.dev APIs from "trigger.dev/sdk/v3" when writing tasks or related utilities
Applied to files:
packages/cli-v3/src/commands/deploy.ts
🧬 Code graph analysis (2)
packages/cli-v3/src/commands/deploy.ts (1)
packages/core/src/v3/schemas/api.ts (4)
InitializeDeploymentRequestBody(414-422)InitializeDeploymentRequestBody(424-424)InitializeDeploymentResponseBody(402-410)InitializeDeploymentResponseBody(412-412)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.ts (2)
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)
deployment(182-200)packages/core/src/v3/schemas/api.ts (2)
GetDeploymentResponseBody(455-487)GetDeploymentResponseBody(489-489)
⏰ 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). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
packages/cli-v3/src/commands/deploy.ts (7)
3-6: LGTM! Clean import organization.The addition of
InitializeDeploymentRequestBodyalongside the existingInitializeDeploymentResponseBodyimport is properly structured and uses type-only imports, which aligns with the TypeScript best practices mentioned in the guidelines.
623-623: Good API optimization with narrowed type signature.Reducing the
failDeployfunction signature to only requireidandshortCodefrom the Deployment object is a solid improvement that follows the principle of least privilege and makes the function more flexible.
739-743: Well-documented workaround with clear future direction.The function signature and documentation clearly explain this is a temporary workaround for the build server workflow. The comments provide excellent context about the current implementation and future plans.
744-780: Robust error handling for attachment flow.The attachment logic properly validates the existing deployment with appropriate error messages. The status check correctly rejects terminal states while allowing ongoing states, which is essential for the build server workflow.
776-779: Clean mapping of imageReference to imageTag.The spread operator with explicit
imageTagmapping handles the schema difference betweenGetDeploymentResponseBody(which hasimageReference) andInitializeDeploymentResponseBody(which expectsimageTag). This maintains type safety while adapting to the existing API contracts.
782-791: Consistent error handling for initialization flow.The new deployment initialization path maintains the same error handling pattern as the existing code, ensuring consistency and proper error propagation.
312-322: Validate or document TRIGGER_EXISTING_DEPLOYMENT_ID before passing it to initializeOrAttachDeploymentrg shows the only occurrence is packages/cli-v3/src/commands/deploy.ts:321 — add explicit validation/coercion to the expected type (or document that undefined is acceptable) before forwarding the env var.
apps/webapp/app/routes/api.v1.deployments.$deploymentId.ts (2)
1-2: Type-only imports: good change.
56-60: Confirm externalBuildData.buildToken exposure; imagePlatform DB default present
imagePlatform: API schema requires a string (GetDeploymentResponseBody.imagePlatform: z.string()) and Prisma + migration add NOT NULL DEFAULT 'linux/amd64' (internal-packages/database/prisma/migrations/20250530131049_add_worker_deployment_image_platform/migration.sql and internal-packages/database/prisma/schema.prisma). If migrations/backfill have been applied, no runtime guard is strictly required; if not, coalesce as a short-term guard: imagePlatform: deployment.imagePlatform ?? "linux/amd64".
externalBuildData: ExternalBuildData includes buildToken (packages/core/src/v3/schemas/api.ts) and the route apps/webapp/app/routes/api.v1.deployments.$deploymentId.ts returns deployment.externalBuildData directly — callers will receive buildToken. CLI and remote-builder consume this token (packages/cli-v3/src/deploy/buildImage.ts, apps/webapp/app/v3/remoteImageBuilder.server.ts). Either confirm this exposure is intended and ensure only appropriately scoped API keys can call the endpoint, or redact/gate buildToken in the response (omit buildToken before returning).
packages/core/src/v3/schemas/api.ts (2)
470-472: Ensure backward compatibility for imagePlatform — add DB default/backfill or allow nullish temporarilyimagePlatform is now required (z.string()); if DB rows contain NULL/undefined the route will fail. Automated search for "imagePlatform" in prisma schema/migrations returned no matches — could not confirm a non-null DB default or backfill. Either add a migration/backfill guaranteeing non-null values or make the schema transitional-nullish until backfill completes.
File: packages/core/src/v3/schemas/api.ts (lines 470-472)
Option A (keep required, validate format):
- imagePlatform: z.string(), + // e.g. "linux/amd64", "linux/arm64" + imagePlatform: z + .string() + .regex(/^[^/]+\/[^/]+$/, 'Expected a Docker platform like "linux/amd64"'),Option B (transitional nullability; remove after backfill):
- imagePlatform: z.string(), + imagePlatform: z.string().nullish(),
471-471: Split credentials out of externalBuildData — do not return buildToken in general GET responses.externalBuildData (packages/core/src/v3/schemas/api.ts) contains buildToken and is included in GetDeploymentResponseBody; it is returned by API routes (apps/webapp/app/routes/api.v1.deployments*.ts) and consumed by CLI/remote-build code (packages/cli-v3/src/deploy/buildImage.ts, packages/cli-v3/src/commands/deploy.ts, packages/cli-v3/src/commands/workers/build.ts, apps/webapp/app/v3/remoteImageBuilder.server.ts).
Action: split ExternalBuildData into metadata (ExternalBuildInfo) and credentials (ExternalBuildCredentials containing buildToken), and stop returning credentials from general GETs — only return them from a scoped/role‑gated endpoint or via short‑lived tokens.
Confirm:
- Which clients actually require the raw buildToken vs just buildId/projectId?
- Is buildToken scoped and short‑lived?
- Are additional role/scope checks needed before including credentials in responses?
myftija
force-pushed
the
attach-to-existing-deployment
branch
from
20e2f31 to
3d564cd
Compare
| errorData: deployment.errorData, | ||
| imagePlatform: deployment.imagePlatform, | ||
| externalBuildData: | ||
| deployment.externalBuildData as GetDeploymentResponseBody["externalBuildData"], |
There was a problem hiding this comment.
Not ideal adding this to the get deployment endpoint, but it won't hurt either. The depot tokens are ephemeral and scoped, the initialize deployment endpoint already returns these.
The alternative would have been to do the matching to the existing deployment in the initialize endpoint instead, by adding a parameter for the existing deployment id. That would make its interface a bit awkward though.
We'll eventually adapt the deployment flow to use the build server as the entry point. We will then also be able to get rid of this workaround.
In the build server we initialize the deployment before installing the project dependencies,
so that the status is correctly reflected in the dashboard. In this case, we need to attach
to the existing deployment and continue with the remote build process.
This is a workaround to avoid major changes in the deploy command and workflow. In the future,
we'll likely make the build server the entry point of the flow for building and deploying and also
adapt the related deployment API endpoints.