feat(deployments): support local builds in cloud #2628
Conversation
🦋 Changeset detectedLatest commit: 17e7130 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 |
|
Warning Rate limit exceeded@myftija has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 10 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughAdds registry credential generation: new API route, platform service call, and DeploymentService method to produce temporary Docker registry credentials. Introduces a --force-local-build CLI flag and threads apiClient/authenticateToRegistry through CLI build flows. Local build path now supports optional Docker login/logout using credentials from environment or the server. Adds skipPushToRegistry request field and a GenerateRegistryCredentialsResponseBody schema. Updates package dependency for platform to a prerelease version. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
♻️ Duplicate comments (1)
apps/webapp/app/v3/services/deployment.server.ts (1)
257-275: Harden ECR host/region parsing; avoid substring checksSubstring matches can misclassify arbitrary strings. Parse the ECR host and extract the region with an anchored regex:
- const getDeploymentRegion = (deployment: Pick<WorkerDeployment, "imageReference">) => { - if (!deployment.imageReference) { - return errAsync({ type: "deployment_has_no_image_reference" as const }); - } - if (!deployment.imageReference.includes("amazonaws.com")) { - return errAsync({ type: "registry_not_supported" as const }); - } - - // we should connect the deployment to a region more explicitly in the future - // for now we just use the image reference to determine the region - if (deployment.imageReference.includes("us-east-1")) { - return okAsync({ region: "us-east-1" as const }); - } - if (deployment.imageReference.includes("eu-central-1")) { - return okAsync({ region: "eu-central-1" as const }); - } - - return errAsync({ type: "registry_region_not_supported" as const }); - }; + const getDeploymentRegion = (deployment: Pick<WorkerDeployment, "imageReference">) => { + if (!deployment.imageReference) { + return errAsync({ type: "deployment_has_no_image_reference" as const }); + } + // <account>.dkr.ecr.<region>.amazonaws.com/<repo>:<tag> + const m = deployment.imageReference.match( + /^(\d{12})\.dkr\.ecr\.(us-east-1|eu-central-1)\.amazonaws\.com\b/i + ); + if (!m) { + return errAsync({ type: "registry_not_supported" as const }); + } + const region = m[2] as "us-east-1" | "eu-central-1"; + return okAsync({ region }); + };This also addresses the “Incomplete URL substring sanitization” alert noted previously.
🧹 Nitpick comments (5)
packages/core/src/v3/schemas/api.ts (1)
442-452: Validate expiresAt format (ISO) for consistencyPrefer explicit timestamp validation to avoid drift across clients. Suggest:
-export const GenerateRegistryCredentialsResponseBody = z.object({ - username: z.string(), - password: z.string(), - expiresAt: z.string(), - repositoryUri: z.string(), -}); +export const GenerateRegistryCredentialsResponseBody = z.object({ + username: z.string(), + password: z.string(), + // RFC 3339/ISO string returned to clients + expiresAt: z.string().datetime(), + repositoryUri: z.string(), +});This keeps the wire format as a string while enforcing shape.
apps/webapp/app/v3/services/deployment.server.ts (1)
277-291: Tidy success check and align expiresAt type
- The platform helper throws on failure; the post-check for !result.success is redundant.
- Consider returning expiresAt as an ISO string (or keep Date and convert at the route) to match the public schema.
- const generateCredentials = ({ region }: { region: "us-east-1" | "eu-central-1" }) => - fromPromise(generateRegistryCredentials(authenticatedEnv.projectId, region), (error) => ({ - type: "other" as const, - cause: error, - })).andThen((result) => { - if (!result || !result.success) { - return errAsync({ type: "missing_registry_credentials" as const }); - } - return okAsync({ - username: result.username, - password: result.password, - expiresAt: new Date(result.expiresAt), - repositoryUri: result.repositoryUri, - }); - }); + const generateCredentials = ({ region }: { region: "us-east-1" | "eu-central-1" }) => + fromPromise(generateRegistryCredentials(authenticatedEnv.projectId, region), (error) => ({ + type: "other" as const, + cause: error, + })).andThen((result) => + okAsync({ + username: result.username, + password: result.password, + // keep ISO string; route can send as-is and clients validate .datetime() + expiresAt: result.expiresAt, + repositoryUri: result.repositoryUri, + }) + );packages/cli-v3/src/commands/deploy.ts (3)
327-330: Derive a single push decision and reuse it (prevents mismatch)If the user doesn’t pass --push/--no-push, pushing should default to true when remote build is explicitly skipped. Also ensure finalize skip matches the actual push.
- const isLocalBuild = options.forceLocalBuild || !deployment.externalBuildData; - // Would be best to actually store this separately in the deployment object. This is an okay proxy for now. - const remoteBuildExplicitlySkipped = options.forceLocalBuild && !!deployment.externalBuildData; + const isLocalBuild = options.forceLocalBuild || !deployment.externalBuildData; + // Remote build existed, but user forced local build + const remoteBuildExplicitlySkipped = options.forceLocalBuild && !!deployment.externalBuildData; + // If forcing local over a remote build, default to pushing unless user explicitly said --no-push + const shouldPushToRegistry = options.push ?? remoteBuildExplicitlySkipped; + if (remoteBuildExplicitlySkipped && !shouldPushToRegistry) { + throw new Error( + "Cannot skip the server registry push without pushing the image locally. Remove --no-push or don’t force a local build." + ); + }
451-455: Tie authenticate/push togetherKeep login only when needed, and align push with the single decision variable.
- push: options.push, - authenticateToRegistry: remoteBuildExplicitlySkipped, + push: shouldPushToRegistry, + authenticateToRegistry: shouldPushToRegistry,Prevents logging in without pushing (or vice versa).
533-539: Align finalize skip with actual push decisionEnsure server skip matches whether the CLI pushed.
- skipPushToRegistry: remoteBuildExplicitlySkipped, + skipPushToRegistry: shouldPushToRegistry,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.changeset/thin-pants-design.md(1 hunks)apps/webapp/app/routes/api.v1.deployments.$deploymentId.generate-registry-credentials.ts(1 hunks)apps/webapp/app/services/platform.v3.server.ts(1 hunks)apps/webapp/app/v3/services/deployment.server.ts(2 hunks)apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts(1 hunks)apps/webapp/package.json(1 hunks)packages/cli-v3/src/apiClient.ts(2 hunks)packages/cli-v3/src/commands/deploy.ts(7 hunks)packages/cli-v3/src/commands/workers/build.ts(1 hunks)packages/cli-v3/src/deploy/buildImage.ts(13 hunks)packages/core/src/v3/schemas/api.ts(2 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:
apps/webapp/app/routes/api.v1.deployments.$deploymentId.generate-registry-credentials.tsapps/webapp/app/services/platform.v3.server.tspackages/core/src/v3/schemas/api.tspackages/cli-v3/src/commands/deploy.tsapps/webapp/app/v3/services/deployment.server.tspackages/cli-v3/src/apiClient.tsapps/webapp/app/v3/services/finalizeDeploymentV2.server.tspackages/cli-v3/src/deploy/buildImage.tspackages/cli-v3/src/commands/workers/build.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:
apps/webapp/app/routes/api.v1.deployments.$deploymentId.generate-registry-credentials.tsapps/webapp/app/services/platform.v3.server.tspackages/core/src/v3/schemas/api.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/v3/services/finalizeDeploymentV2.server.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.generate-registry-credentials.tsapps/webapp/app/services/platform.v3.server.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/v3/services/finalizeDeploymentV2.server.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.generate-registry-credentials.tsapps/webapp/app/services/platform.v3.server.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/v3/services/finalizeDeploymentV2.server.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.generate-registry-credentials.tsapps/webapp/app/services/platform.v3.server.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/v3/services/finalizeDeploymentV2.server.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:
apps/webapp/package.json
🧬 Code graph analysis (6)
apps/webapp/app/routes/api.v1.deployments.$deploymentId.generate-registry-credentials.ts (3)
apps/webapp/app/services/apiAuth.server.ts (1)
authenticateRequest(379-441)packages/core/src/v3/schemas/api.ts (4)
ProgressDeploymentRequestBody(381-385)ProgressDeploymentRequestBody(387-387)GenerateRegistryCredentialsResponseBody(442-447)GenerateRegistryCredentialsResponseBody(449-451)apps/webapp/app/v3/services/deployment.server.ts (1)
DeploymentService(12-323)
apps/webapp/app/services/platform.v3.server.ts (2)
apps/webapp/app/v3/services/deployment.server.ts (1)
generateRegistryCredentials(244-297)packages/core/src/v3/apiClientManager/index.ts (1)
client(57-63)
apps/webapp/app/v3/services/deployment.server.ts (2)
apps/webapp/app/v3/services/failDeployment.server.ts (1)
FINAL_DEPLOYMENT_STATUSES(8-13)apps/webapp/app/services/platform.v3.server.ts (1)
generateRegistryCredentials(514-530)
packages/cli-v3/src/apiClient.ts (2)
packages/core/src/v3/apiClient/core.ts (1)
wrapZodFetch(727-767)packages/core/src/v3/schemas/api.ts (2)
GenerateRegistryCredentialsResponseBody(442-447)GenerateRegistryCredentialsResponseBody(449-451)
apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts (1)
apps/webapp/app/v3/services/finalizeDeployment.server.ts (1)
FinalizeDeploymentService(15-137)
packages/cli-v3/src/deploy/buildImage.ts (1)
packages/cli-v3/src/apiClient.ts (1)
CliApiClient(56-780)
⏰ 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 (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 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 (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (19)
apps/webapp/package.json (1)
120-120: Verify the prerelease platform version before merging.The dependency uses a prerelease version (
0.0.0-prerelease-ecr-20251021203336), which suggests this is a temporary development version. Ensure this is replaced with a stable release version before merging to production.apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts (1)
74-81: LGTM!The refactor to instantiate
FinalizeDeploymentServiceearly and reuse it in both the skip path and normal flow is clean and avoids duplication. The early return whenskipPushToRegistryis true provides a clear bypass of the registry push logic.packages/cli-v3/src/deploy/buildImage.ts (4)
25-25: LGTM!The new
authenticateToRegistryandapiClientoptions are appropriately typed and align with the registry authentication feature requirements.Also applies to: 44-44
428-484: LGTM!The registry authentication flow is well-implemented with proper error handling:
- Validates registry host extraction
- Handles credential retrieval failures gracefully
- Properly writes password to stdin for docker login
- Collects logs for debugging
- Provides clear success/failure feedback
545-548: LGTM!The logout logic properly handles cleanup in both success and error paths, ensuring credentials are not left in the docker config.
Also applies to: 595-598
921-929: Verify registry host extraction for edge cases.The
extractRegistryHostFromImageTagfunction assumes the registry host is the first segment and contains a dot. This may not handle all edge cases:
- Local registries like
localhost:5000/image(no dot in host)- Registry hosts with non-standard formats
Since the code also checks
TRIGGER_DOCKER_REGISTRYenvironment variable first (line 432), ensure the documentation clarifies how to handle non-standard registry hosts using that variable.apps/webapp/app/services/platform.v3.server.ts (1)
514-530: LGTM!The
generateRegistryCredentialsfunction follows the established patterns in this file:
- Guards against missing client
- Includes proper error logging
- Throws on failure for caller handling
- Region type is appropriately constrained to supported values
packages/cli-v3/src/apiClient.ts (1)
331-345: LGTM!The
generateRegistryCredentialsmethod follows established patterns inCliApiClient:
- Proper access token validation
- Correct use of
wrapZodFetchwith the schema- Appropriate HTTP method and endpoint
- Consistent error handling
packages/cli-v3/src/commands/workers/build.ts (1)
339-339: LGTM!The addition of
apiClient: projectClient.clientcorrectly provides the API client instance tobuildImage, enabling the registry authentication flow.apps/webapp/app/routes/api.v1.deployments.$deploymentId.generate-registry-credentials.ts (5)
16-19: LGTM!The POST method enforcement is standard and returns the appropriate 405 status for unsupported methods.
27-36: LGTM!The authentication configuration correctly restricts access to API key authentication only, which is appropriate for a deployment-related endpoint.
41-46: Review potentially unused request body parsing.The request body is parsed and validated against
ProgressDeploymentRequestBody, but the result doesn't appear to be used. ThegenerateRegistryCredentialsservice method only requiresauthenticatedEnvanddeploymentIdparameters.If the body parsing is not needed for this endpoint, consider removing it to simplify the code. If it's intended for future use or consistency with other endpoints, a comment explaining the rationale would be helpful.
48-61: LGTM!The service call and success response handling are well-implemented:
- Proper use of the Result pattern with
match()- Response correctly satisfies
GenerateRegistryCredentialsResponseBody- Appropriate conversion of
expiresAtto ISO string format- Correct 200 status code
62-98: LGTM!The error handling is comprehensive and well-structured:
- Each service error type is mapped to an appropriate HTTP status
- Important errors are logged for debugging
- The
satisfiescheck ensures exhaustive error handling- Clear error messages returned to the client
packages/core/src/v3/schemas/api.ts (1)
373-377: Clarify and verify skipPushToRegistry semanticsConfirm server treats skipPushToRegistry as “CLI already pushed image; server must not push.” Ensure finalize path won’t attempt a push when true, and that the CLI sets it only when it actually pushed.
packages/cli-v3/src/commands/deploy.ts (4)
60-60: New flag wiring looks fineforceLocalBuild option added; no issues.
131-133: CLI flag addition LGTMHidden --force-local-build flag registered correctly.
400-404: Status label suffixes LGTMConditional local build labels controlled by TRIGGER_LOCAL_BUILD_LABEL_DISABLED; looks good.
431-431: Passing apiClient is appropriateEnables authenticated operations during local builds.
myftija
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli-v3/src/deploy/buildImage.ts(13 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/cli-v3/src/deploy/buildImage.ts
⏰ 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 / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
packages/cli-v3/src/deploy/buildImage.ts (7)
25-25: LGTM: Clean interface extension.The new
authenticateToRegistryandapiClientproperties are well-integrated into the existing options structure.Also applies to: 44-44
926-951: LGTM: Secure credential handling with appropriate fallbacks.The function properly prioritizes environment variables for local testing, then falls back to API-generated credentials. Credentials are never logged, and the error handling is clear.
916-924: Registry host extraction is appropriate for cloud deployments.The heuristic (checking for "." in the host) works well for cloud registries. The
TRIGGER_DOCKER_REGISTRYenvironment variable at line 432 provides an override for edge cases likelocalhost:5000.
540-543: LGTM: Proper registry logout on both success and error paths.The cleanup is properly awaited and conditionally executed, ensuring the registry session is closed regardless of build outcome.
Also applies to: 590-593
84-106: LGTM: Clean parameter threading to local build path.The new authentication parameters are properly threaded through to
localBuildImagealongside existing options.
526-530: Consistent API usage for process options.The change to use
nodeOptionswrapper aligns with the tinyexec usage pattern seen elsewhere in the file (lines 462, 572).
428-428: Appropriate logging levels for registry operations.Debug logs for internal operations (login/logout) and user-facing success messages strike the right balance. The early initialization of the
errorsarray properly captures any registry authentication failures.Also applies to: 455-455, 478-478
myftija
force-pushed
the
local-build-deployments
branch
from
f18ba26 to
af52741
Compare
2 participants
This PR adds support for deploying locally built images to cloud. It uses the
same path as the self-hosting flow and adds the necessary adaptations for
authenticating and pushing to the cloud registry.