Fix broken cloud deploys by using depot ephemeral registry, skip the registry proxy #1637
Conversation
🦋 Changeset detectedLatest commit: 9339d36 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/webapp/app/env.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces significant changes to the deployment process in the Trigger.dev platform. The modifications focus on improving cloud deployments by implementing a depot ephemeral registry approach. Key updates include adding new environment configuration options, updating the deployment finalization service, introducing a new registry proxy mechanism, and modifying the CLI's deployment logic. The changes aim to provide more flexible and robust deployment capabilities, with enhanced error handling and registry management. Changes
Sequence DiagramsequenceDiagram
participant CLI as Trigger.dev CLI
participant WebApp as Deployment API
participant Registry as Depot Registry
participant Worker as Deployment Worker
CLI->>WebApp: Initiate Deployment
WebApp->>WebApp: Validate Deployment
alt Registry Proxy Enabled
WebApp->>Registry: Authenticate
Registry-->>WebApp: Authentication Confirmed
end
WebApp->>Worker: Start Deployment
Worker->>Registry: Push Image
Registry-->>Worker: Image Push Confirmed
Worker->>WebApp: Finalize Deployment
WebApp->>WebApp: Update Deployment Status
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
apps/webapp/app/v3/registryProxy.server.ts (1)
Line range hint
466-482: Enhance temporary file security in streamRequestBodyToTempFile.The temporary file handling could be improved to:
- Use more secure file permissions
- Ensure reliable cleanup even in case of process crashes
async function streamRequestBodyToTempFile(request: IncomingMessage): Promise<string | undefined> { try { const tempDir = await mkdtemp(`${tmpdir()}/`); const tempFilePath = `${tempDir}/requestBody.tmp`; - const writeStream = createWriteStream(tempFilePath); + const writeStream = createWriteStream(tempFilePath, { + mode: 0o600, // Restrict file permissions to owner only + }); await pipeline(request, writeStream); + // Register cleanup on process events + const cleanup = () => { + try { + unlinkSync(tempFilePath); + } catch (error) { + logger.error("Failed to cleanup temp file", { + error: error instanceof Error ? error.message : error, + tempFilePath, + }); + } + }; + process.on('exit', cleanup); + process.on('SIGINT', cleanup); + process.on('SIGTERM', cleanup); return tempFilePath; } catch (error) {
🧹 Nitpick comments (12)
apps/webapp/app/routes/api.v2.deployments.$deploymentId.finalize.ts (2)
38-38: Validate request body for optional fields
While parsing withFinalizeDeploymentRequestBody.safeParse, consider logging or handling optional fields likeskipRegistryProxyto confirm if they're present/absent as expected, which might help debugging.
44-64: Robust error handling
The try-catch block is well-structured. You have distinct handling forServiceValidationErrorvs. generalError, returning 400 and 500 respectively. Consider returning a more descriptive error payload, e.g., error codes or fields to help clients differentiate between potential error states.- return json({ error: error.message }, { status: 400 }); + return json({ + error: error.message, + type: "VALIDATION_ERROR" + }, { status: 400 });apps/webapp/app/v3/services/finalizeDeploymentV2.ts (6)
12-45: Class constructor is missing
You might consider adding a constructor toFinalizeDeploymentV2Servicefor improved testability or if any service-wide initialization steps are needed.
114-131: Enumerate required vs. optional fields
In yourExecutePushToRegistryOptions, consider marking required vs. optional keys explicitly. This can help new contributors or tooling to catch missing fields earlier.
145-216: Surface CLI logs more meaningfully
Currently all logs fordepot pushare appended tostderr. You might parse them further, grouping warnings separately from normal logs, for better observability.
218-237: Consider reusing login session
ensureLoggedIntoDockerRegistry()always writes a fresh config in a new temp directory. If your environment allows, reusing or caching session credentials could speed up subsequent image pushes.
239-249: Add logging for creation
createTempDir()is a potential point of failure if the OS denies permissions. Consider a debug-level log or error handling for improved insight in production.
254-259: Avoid line trimming if logs have meaningful whitespace
If you need to preserve indentation or formatting, be mindful when trimming. If whitespace is irrelevant, your approach is fine.apps/webapp/app/v3/services/finalizeDeployment.server.ts (1)
65-65: Highlight final image reference
StoringimageReferenceinto the database is crucial for traceability. Consider logging or auditing changes for investigations or future references.apps/webapp/app/env.server.ts (1)
150-150: Consider parsing ENABLE_REGISTRY_PROXY as a boolean.Currently,
ENABLE_REGISTRY_PROXYisz.string().optional(). If it's supposed to be interpreted as a boolean (e.g.,"true"/"false"), consider updating toz.enum(["true", "false"]).optional()orz.boolean().optional()to avoid confusion.packages/core/src/v3/schemas/api.ts (1)
225-225: Consider providing a default.Introducing an optional boolean is a good approach, but to avoid potential undefined checks, consider defaulting
skipRegistryProxytofalseor to any sensible default if skipping is only an opt-in scenario.apps/webapp/app/v3/registryProxy.server.ts (1)
438-441: Consider using case-insensitive string comparison.The current implementation might miss "FALSE" or "False" values. Consider using case-insensitive comparison for more robust handling.
- if (!env.ENABLE_REGISTRY_PROXY || env.ENABLE_REGISTRY_PROXY === "false") { + if (!env.ENABLE_REGISTRY_PROXY || env.ENABLE_REGISTRY_PROXY.toLowerCase() === "false") {
📜 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 (13)
.changeset/pink-mice-battle.md(1 hunks)apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/routes/api.v2.deployments.$deploymentId.finalize.ts(2 hunks)apps/webapp/app/v3/registryProxy.server.ts(1 hunks)apps/webapp/app/v3/services/finalizeDeployment.server.ts(3 hunks)apps/webapp/app/v3/services/finalizeDeploymentV2.ts(1 hunks)apps/webapp/app/v3/services/startDeploymentIndexing.server.ts(0 hunks)apps/webapp/package.json(1 hunks)packages/cli-v3/package.json(1 hunks)packages/cli-v3/src/apiClient.ts(1 hunks)packages/cli-v3/src/commands/deploy.ts(3 hunks)packages/cli-v3/src/deploy/buildImage.ts(2 hunks)packages/core/src/v3/schemas/api.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/v3/services/startDeploymentIndexing.server.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (22)
apps/webapp/app/routes/api.v2.deployments.$deploymentId.finalize.ts (2)
6-7: Imports aligned with new service approach
These imports forServiceValidationErrorandFinalizeDeploymentV2Serviceare consistent with the new finalization flow. This is good since it cleanly separates concerns compared to the old indexing service.
2-2: Consider versioning or referencing the correct schema path explicitly
ImportingFinalizeDeploymentRequestBodyfrom@trigger.dev/core/v3looks correct, but ensure any future schema revisions don't break this dependency.Are there references to an older version of this schema anywhere else? We can confirm with a quick repository scan:
✅ Verification successful
Schema versioning is correctly implemented
The import ofFinalizeDeploymentRequestBodyfrom@trigger.dev/core/v3is consistent with the codebase-wide pattern where all deployment-related schemas are centralized in v3, even when used by v1 and v2 API routes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg -A 3 "StartDeploymentIndexingRequestBody"Length of output: 1240
Script:
#!/bin/bash # Search for FinalizeDeploymentRequestBody echo "=== Searching for FinalizeDeploymentRequestBody ===" rg -A 3 "FinalizeDeploymentRequestBody" # Search for deployment-related schemas in v1/v2 paths echo -e "\n=== Searching for deployment schemas in v1/v2 paths ===" rg -g "**/v1/**" -g "**/v2/**" "DeploymentRequestBody|DeploymentResponseBody" # Search for import statements with deployment and version numbers echo -e "\n=== Searching for versioned deployment imports ===" rg "import.*[Dd]eployment.*(@trigger\.dev/core/(v1|v2|v3))"Length of output: 6499
apps/webapp/app/v3/services/finalizeDeploymentV2.ts (5)
46-79: Clear error messages
The error messages for missing or invalid data (lines 48, 49, 53, 61, 65, 73, 77) are easy to understand. Good job! This approach helps isolate each cause of failure.
80-98: Ensure partial deploy states are handled
Pushing to the registry can fail mid-process. Confirm that partially uploaded images or resources are cleaned up or retried if you plan to handle ephemeral states.
99-111: Consistent usage of finalization
Using the olderFinalizeDeploymentServiceafter a successful push is a clean handoff. Verify that skipping the registry proxy or ephemeral deployments do not require additional adjustments in the older service.
133-144: Structured typing for push results
Using discriminated unions (ok: true | false) is an excellent approach to type safety. Great job ensuring a consistent return shape.
1-11: Confirm environment variables exist for all new imports
Ensure that all newly imported modules (@depot/cli,~/env.server, etc.) have the needed environment configuration in production. Missing or invalid env vars might cause unexpected runtime failures.✅ Verification successful
Environment variable configuration is properly implemented ✅
The codebase already has proper environment variable definitions and validation for all new imports. The implementation includes defensive checks and appropriate default values where needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check references to env variables rg "DEPLOY_REGISTRY|DEPOT_TOKEN"Length of output: 2356
apps/webapp/app/v3/services/finalizeDeployment.server.ts (2)
19-19: findFirst vs. findUnique
Switching tofindFirstcan introduce edge cases if multiple deployments share the samefriendlyId. Validate that your schema enforces uniqueness or that returning the first matching record is indeed the desired behavior.
51-56: Skip registry proxy logic
The usage ofregistryProxy.rewriteImageReference(...)is guarded byselfHosted !== trueandskipRegistryProxy !== true. This is a helpful explicit condition. Be sure that ignoring the proxy is acceptable for all self-hosted or ephemeral use cases.packages/cli-v3/src/apiClient.ts (1)
257-257: Potential schema mismatch with the finalize deployment call.You're using
FailDeploymentResponseBodyfor thefinalizeDeploymentmethod. Verify whether that’s intentional or if a dedicated schema (e.g.,FinalizeDeploymentResponseBody) is required.apps/webapp/app/env.server.ts (1)
152-153: Validate usage of registry credentials in environment variables.Storing credentials like
DEPLOY_REGISTRY_USERNAMEandDEPLOY_REGISTRY_PASSWORDin env variables is common, but ensure they are not inadvertently exposed in logs or error messages for security reasons.packages/cli-v3/src/commands/deploy.ts (5)
3-3: Good addition for typed responses.Importing
InitializeDeploymentResponseBodyimproves clarity for deployment initialization responses.
5-6: Imports for file path and commands are appropriate.
resolveandxfromtinyexecare used effectively for path resolution and command execution. Nice additions.
10-10: Approved usage of resolveAlwaysExternal.This import clarifies forced external dependencies. Keep an eye on performance implications if the external list grows large.
19-19: buildImage import looks correct.This import is part of the typical deployment workflow; no issues found.
427-427: Eliminate the registry prefix when not self-hosted.This allows a simpler image reference for cloud builds. Confirm that
buildResult.imagealways includes a valid reference during non-self-hosted builds.packages/cli-v3/src/deploy/buildImage.ts (2)
198-198: Correctly swapping--pushfor--save.Using
--savesuits the depot ephemeral registry workflow better. If you still need to push images externally, ensure the process includes separate steps or flags.
250-250: Hardcoded depot registry reference.Using
registry.depot.dev/${options.buildProjectId}:${options.buildId}is aligned with the ephemeral registry approach. Just confirm it won’t conflict with future expansions for custom registries..changeset/pink-mice-battle.md (1)
1-6: Release notes look good.This changeset succinctly captures the patch for both packages, describing the ephemeral registry fix.
packages/cli-v3/package.json (1)
77-77: Verify the new CLI version.Ensure that
0.0.1-cli.2.80.0is compatible with your deployment flow and free of known issues.✅ Verification successful
@depot/cli version 0.0.1-cli.2.80.0 appears stable and compatible
The version is consistently integrated across the project with no reported issues or error patterns in critical deployment flows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for known issues with the specified version across the repo and logs. rg -A 6 '@depot/cli'Length of output: 8514
Script:
#!/bin/bash # Check for any issues, errors, or warnings related to @depot/cli rg -i "(\berror\b|\bissue\b|\bwarn\b|\bfail\b).*@depot/cli" -A 3 # Check commit messages for any mentions of depot CLI issues git log --grep="@depot/cli" --grep="depot.*cli" -i --since="6 months ago" --pretty=format:"%h %s"Length of output: 66
apps/webapp/package.json (1)
49-49: Addition of @depot/cli dependency.This looks aligned with the CLI update in
packages/cli-v3. Nice consistency across the packages.apps/webapp/app/v3/registryProxy.server.ts (1)
438-441: Add type safety for ENABLE_REGISTRY_PROXY environment variable.The environment variable should be properly typed in the environment schema to ensure type safety and documentation.
Let's verify if the environment variable is properly typed in the environment schema:
| async function writeJSONFile(path: string, json: any, pretty = false) { | ||
| await writeFile(path, JSON.stringify(json, undefined, pretty ? 2 : undefined), "utf8"); | ||
| } |
There was a problem hiding this comment.
Secure JSON writing
writeJSONFile() writes credentials to disk in plaintext. If logs or backups capture these files, a leak is possible. Consider in-memory solutions or ephemeral encryption.
- await writeFile(path, JSON.stringify(json, undefined, pretty ? 2 : undefined), "utf8");
+ // Potential improvement: ephemeral in-memory store or encrypt the file
+ // If truly necessary to write to disk, ensure to limit file permissions.Committable suggestion skipped: line range outside the PR's diff.
@trigger.dev/build
@trigger.dev/rsc
@trigger.dev/react-hooks
@trigger.dev/sdk
@trigger.dev/core
trigger.dev
commit: |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
@depot/clidependency to version 0.0.1-cli.2.80.0Performance