Fix: fixes and prerequisites for v4 self-hosting #2150
Conversation
🦋 Changeset detectedLatest commit: 49f63b7 The changes in this PR will be included in the next version bump. 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 |
WalkthroughThis update introduces a range of changes across multiple components of the codebase. Environment variable handling and configuration schemas were refactored, particularly for Docker and Kubernetes settings, including renaming, regrouping, and consolidating related variables. Machine preset overrides are now supported via a JSON file, with new logic to read and validate overrides and apply them to platform machine configurations. The bootstrap process for worker groups was added to the webapp, enabling automated initialization based on environment variables. Error handling for presigned URL generation was improved, with structured error responses replacing simple null or undefined checks. The project creation flow was simplified by removing the v3 feature flag, and related logic and admin controls were updated accordingly. Several files had imports updated to reference the new machine preset logic. Docker-related build and configuration files were optimized, with caching added to Dockerfile and unused pgAdmin configuration removed. User-facing prompts and error messages were also refined. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (25)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 4
🔭 Outside diff range comments (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx (1)
165-169: 💡 Verification agent🧩 Analysis chain
Hidden input approach removes user choice complexity.
Replacing the version selection dropdown with hidden inputs based on organization capability simplifies the UI and ensures consistent project version selection based on organization permissions.
🏁 Script executed:
#!/bin/bash # Description: Verify that v3Enabled feature flag has been completely removed from the codebase # Expected: No remaining references to v3Enabled feature flag should exist echo "Checking for remaining v3Enabled feature flag references..." rg "v3Enabled.*flag|features.*v3Enabled" --type ts --type tsx --type js --type jsxLength of output: 241
Let’s re-run the search with proper file globs to catch all TS, TSX, JS, and JSX files:
🏁 Script executed:
#!/bin/bash echo "Searching for any 'v3Enabled' references across TS, TSX, JS, and JSX files..." rg "v3Enabled" --glob "*.{ts,tsx,js,jsx}" -n || trueLength of output: 1155
Remove all remaining
v3Enabledfeature-flag referencesOur search shows that
v3Enabledis still used in multiple places and needs to be cleaned up or replaced with the newcanCreateV3Projectslogic:• apps/webapp/app/models/project.server.ts:38
• apps/webapp/app/models/organization.server.ts:69
• apps/webapp/app/routes/app.orgs.$organizationSlug.projects.new/route.tsx:43, 64, 78, 126
• apps/webapp/app/routes/admin.orgs.tsx:105
• apps/webapp/app/routes/app.orgs.$organizationSlug.select-plan/route.tsx:34
• apps/webapp/app/models/admin.server.ts:130Please update or remove each of these references so that project-version gating relies solely on
canCreateV3Projects(or another consolidated capability check) before we approve.
🧹 Nitpick comments (4)
CONTRIBUTING.md (1)
95-108: Good simplification of setup process.Removing the pgAdmin and v3Enabled setup steps aligns with the PR objectives and simplifies the development workflow. The step renumbering is correct.
Consider addressing these minor formatting improvements:
-1. Visit http://localhost:3030 in your browser and create a new V3 project called "v3-catalog". +1. Visit http://localhost:3030 in your browser, and create a new V3 project called "v3-catalog". -2. In Postgres go to the "Projects" table and for the project you create change the `externalRef` to `yubjwjsfkxnylobaqvqz`. +2. In Postgres, go to the "Projects" table and for the project you create change the `externalRef` to `yubjwjsfkxnylobaqvqz`.Also consider using a markdown link for the URL:
-1. Visit http://localhost:3030 in your browser, and create a new V3 project called "v3-catalog". +1. Visit [http://localhost:3030](http://localhost:3030) in your browser, and create a new V3 project called "v3-catalog".🧰 Tools
🪛 LanguageTool
[uncategorized] ~97-~97: A comma might be missing here.
Context: ... V3 project called "v3-catalog". 2. In Postgres go to the "Projects" table and for the ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🪛 markdownlint-cli2 (0.17.2)
95-95: Bare URL used
null(MD034, no-bare-urls)
apps/supervisor/src/workerToken.ts (1)
4-21: Consider using async file operations to prevent blocking.Synchronous file operations can block the event loop, especially if the file is on a slow filesystem or network mount. Consider using the async version for better performance.
+import { readFile } from "fs/promises"; -export function getWorkerToken() { +export async function getWorkerToken() { if (!env.TRIGGER_WORKER_TOKEN.startsWith("file://")) { return env.TRIGGER_WORKER_TOKEN; } const tokenPath = env.TRIGGER_WORKER_TOKEN.replace("file://", ""); console.debug( JSON.stringify({ message: "🔑 Reading worker token from file", tokenPath, }) ); - const token = readFileSync(tokenPath, "utf8"); + const token = await readFile(tokenPath, "utf8"); return token; }Note: This would require updating the caller in
index.tsto await the function call.apps/webapp/app/bootstrap.ts (1)
41-64: Consider security implications of token exposure in console output.The bootstrap function logs the plaintext token to the console, which could be captured in logs. While this is intended for initial setup, consider if there are additional security measures needed.
Consider adding a warning about log security:
+WARNING: The token below will be logged. Ensure your logs are secure. + Worker group: ${workerGroup.name}apps/webapp/app/v3/r2.server.ts (1)
234-234: Remove unused variable assignment.There's an unused assignment
signed;on line 234 that should be removed.- signed; - return {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
.changeset/smooth-planets-flow.md(1 hunks)CONTRIBUTING.md(2 hunks)apps/supervisor/.env.example(0 hunks)apps/supervisor/src/env.ts(2 hunks)apps/supervisor/src/index.ts(3 hunks)apps/supervisor/src/workerToken.ts(1 hunks)apps/supervisor/src/workloadManager/docker.ts(4 hunks)apps/supervisor/src/workloadManager/kubernetes.ts(1 hunks)apps/webapp/app/bootstrap.ts(1 hunks)apps/webapp/app/entry.server.tsx(2 hunks)apps/webapp/app/env.server.ts(3 hunks)apps/webapp/app/features.server.ts(0 hunks)apps/webapp/app/hooks/useFeatures.ts(1 hunks)apps/webapp/app/models/admin.server.ts(0 hunks)apps/webapp/app/models/organization.server.ts(1 hunks)apps/webapp/app/models/project.server.ts(0 hunks)apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts(2 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx(3 hunks)apps/webapp/app/routes/api.v1.packets.$.ts(2 hunks)apps/webapp/app/routes/engine.v1.dev.runs.$runFriendlyId.snapshots.$snapshotFriendlyId.attempts.start.ts(1 hunks)apps/webapp/app/routes/resources.packets.$environmentId.$.ts(1 hunks)apps/webapp/app/services/platform.v3.server.ts(3 hunks)apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts(1 hunks)apps/webapp/app/v3/machinePresets.server.ts(1 hunks)apps/webapp/app/v3/r2.server.ts(3 hunks)apps/webapp/app/v3/runEngine.server.ts(1 hunks)apps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts(1 hunks)docker/Dockerfile(1 hunks)docker/docker-compose.yml(0 hunks)docker/pgadmin/servers.json(0 hunks)packages/cli-v3/src/commands/switch.ts(1 hunks)packages/core/src/v3/runEngineWorker/supervisor/session.ts(1 hunks)
💤 Files with no reviewable changes (6)
- apps/supervisor/.env.example
- docker/docker-compose.yml
- apps/webapp/app/features.server.ts
- apps/webapp/app/models/project.server.ts
- apps/webapp/app/models/admin.server.ts
- docker/pgadmin/servers.json
🧰 Additional context used
🧬 Code Graph Analysis (9)
apps/webapp/app/entry.server.tsx (1)
apps/webapp/app/bootstrap.ts (1)
bootstrap(8-19)
apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts (1)
apps/webapp/app/v3/r2.server.ts (1)
generatePresignedUrl(210-240)
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (1)
apps/webapp/app/env.server.ts (1)
env(793-793)
apps/supervisor/src/workerToken.ts (1)
apps/supervisor/src/env.ts (1)
env(100-100)
apps/webapp/app/routes/api.v1.packets.$.ts (2)
apps/webapp/app/v3/r2.server.ts (1)
generatePresignedUrl(210-240)packages/core/src/v3/apps/http.ts (1)
json(65-75)
apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx (1)
apps/webapp/app/features.server.ts (1)
featuresForRequest(22-25)
apps/webapp/app/v3/r2.server.ts (2)
apps/webapp/app/env.server.ts (1)
env(793-793)apps/webapp/app/models/api-key.server.ts (1)
envSlug(89-104)
apps/supervisor/src/env.ts (1)
apps/supervisor/src/envUtil.ts (1)
BoolEnv(6-12)
apps/supervisor/src/index.ts (2)
apps/supervisor/src/env.ts (1)
env(100-100)apps/supervisor/src/workerToken.ts (1)
getWorkerToken(4-21)
🪛 LanguageTool
CONTRIBUTING.md
[uncategorized] ~97-~97: A comma might be missing here.
Context: ... V3 project called "v3-catalog". 2. In Postgres go to the "Projects" table and for the ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🪛 markdownlint-cli2 (0.17.2)
CONTRIBUTING.md
95-95: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 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 (1, 8)
- 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 (45)
packages/cli-v3/src/commands/switch.ts (1)
87-87: Update prompt wording to clarify default profile selection.The updated message
"Select a new default profile"more clearly conveys that this profile will be set as the default. Ensure consistency with other CLI prompts and any related documentation or tests..changeset/smooth-planets-flow.md (1)
1-6: Confirm changeset metadata accuracy.The new changeset entry correctly records a patch-level update with the summary "Update profile switcher". No issues with formatting or metadata.
docker/Dockerfile (1)
29-29: Leverage build cache for pnpm and Prisma steps.The added
--mount=type=cache,id=pnpm,target=/root/.local/share/pnpm/storedirectives for development, production, and Prisma generation significantly speed up subsequent builds. This change aligns with best practices for Docker layer caching.Also applies to: 37-37, 41-41
apps/webapp/app/v3/runEngine.server.ts (1)
2-2: Switch import to new platform service module.The import of
defaultMachinenow points to~/services/platform.v3.server, enabling machine preset overrides. Verify that the module exportsdefaultMachineas expected and that there are no unresolved import paths in the build.apps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts (1)
32-32: Align import ofdefaultMachinewith updated platform service.Updated the import to
~/services/platform.v3.serverto leverage the new override logic. Ensure this change is reflected across all related services and thatdefaultMachineis correctly resolved at runtime.apps/webapp/app/v3/machinePresets.server.ts (1)
2-2:✅ Verification successful
Verify the import interface consistency.
The import source change from the platform package to a local service looks good and aligns with the machine preset override functionality. Ensure that the exported
defaultMachineandmachinesfrom the new service maintain the same interface and behavior as the original platform exports.
🏁 Script executed:
#!/bin/bash # Description: Verify that the new service exports match the expected interface # Expected: defaultMachine and machines should be exported with correct types # Check the new service file exists and exports the required items rg -A 10 "export.*defaultMachine|export.*machines" apps/webapp/app/services/platform.v3.server.ts # Check for any other files that import from the old path rg "@trigger.dev/platform/v3.*defaultMachine|@trigger.dev/platform/v3.*machines"Length of output: 543
Verification complete: new service exports and no stale imports
Verified that
apps/webapp/app/services/platform.v3.server.tsexports bothdefaultMachineandmachines, and there are no remaining imports from@trigger.dev/platform/v3. The interface remains consistent with the original platform exports. Approving the change.apps/webapp/app/routes/engine.v1.dev.runs.$runFriendlyId.snapshots.$snapshotFriendlyId.attempts.start.ts (1)
9-9: Consistent with machine preset refactoring.This import change is consistent with the machine preset centralization effort seen in other files. The change maintains the same functionality while enabling override capabilities.
packages/core/src/v3/runEngineWorker/supervisor/session.ts (1)
171-171: Good formatting improvement.Adding the space after the closing bracket improves the readability of the error message and follows standard formatting conventions.
apps/webapp/app/entry.server.tsx (2)
18-18: Clean integration of bootstrap functionality.The import follows the established pattern in this file and aligns with the conditional worker group initialization described in the PR objectives.
181-183: Proper error handling for bootstrap initialization.The bootstrap call follows the same error handling pattern as
Worker.init(), ensuring consistent error logging during server startup.apps/webapp/app/models/organization.server.ts (1)
69-69: Simplified v3 enablement logic aligns with feature flag removal.The change from a compound condition to simply
!features.isManagedCloudreflects the removal of the globalv3Enabledfeature flag mentioned in the PR objectives. This simplification makes the logic more direct and easier to understand.apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (1)
863-863: Improved URL construction with trailing slash handling.The change to remove trailing slashes before appending
/otelensures proper URL formation regardless of theAPP_ORIGINformat. This defensive approach prevents potential double-slash issues in the OTEL endpoint URL.apps/supervisor/src/index.ts (3)
27-27: Good abstraction for worker token retrieval.The import of
getWorkerTokenenables support for reading worker tokens from files when the environment variable uses thefile://prefix, which aligns with the PR's objective to improve self-hosting flexibility.
123-123: Consistent use of abstracted token retrieval.Using
getWorkerToken()instead of direct environment variable access provides a clean abstraction that supports both direct token values and file-based token sources.
71-71: Environment variable name update aligns with restructuring.The change from
RUNNER_DOCKER_AUTOREMOVEtoDOCKER_AUTOREMOVE_EXITED_CONTAINERSreflects the environment variable reorganization and prefixing mentioned in the PR objectives for improved consistency.apps/supervisor/src/workloadManager/kubernetes.ts (1)
239-239:✅ Verification successful
LGTM: Environment variable renaming improves consistency.
The renaming of ephemeral storage environment variables to include the
KUBERNETES_prefix aligns with the broader reorganization of Kubernetes-related environment variables. This improves clarity and grouping.Please verify that the new environment variable names are properly defined in the environment configuration:
Also applies to: 245-245
🏁 Script executed:
#!/bin/bash # Description: Verify the new Kubernetes environment variables are defined in env configuration # Check for the new environment variable definitions in env files rg -A 2 -B 2 "KUBERNETES_EPHEMERAL_STORAGE_SIZE" # Check if the old environment variables are still referenced anywhere rg "EPHEMERAL_STORAGE_SIZE_(REQUEST|LIMIT)" --type tsLength of output: 2279
LGTM: New Kubernetes ephemeral storage variables are correctly defined
- Verified that
KUBERNETES_EPHEMERAL_STORAGE_SIZE_REQUESTandKUBERNETES_EPHEMERAL_STORAGE_SIZE_LIMITare declared inapps/supervisor/src/env.tswith appropriate defaults.- Confirmed no references to the old unprefixed
EPHEMERAL_STORAGE_SIZE_REQUEST/LIMITremain in the supervisor code.- References to
POD_EPHEMERAL_STORAGE_SIZE_REQUEST/LIMITinapps/kubernetes-provider/src/index.tsuse a different prefix and are unaffected by this rename.apps/webapp/app/routes/resources.packets.$environmentId.$.ts (2)
44-46: Excellent improvement to error handling.The update to use
signed.successwith explicit error messaging is much better than the previous simple truthy check. This provides clearer error reporting and structured handling of presigned request generation failures.
48-49: LGTM: Updated to use structured request object.The change to access
signed.request.urlandsigned.request.headersaligns with the new discriminated union return type fromgeneratePresignedRequest, providing better type safety and structure.apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts (2)
120-134: Excellent error handling improvement with detailed logging.The update to use structured success/error checking is much better than the previous approach. The detailed error logging with task run ID and payload context will significantly help with debugging presigned URL generation issues.
149-163: Consistent error handling pattern for output URLs.Good consistency with the payload handling above. The structured error handling and detailed logging will provide valuable debugging information when presigned URL generation fails for output data.
apps/webapp/app/routes/api.v1.packets.$.ts (3)
28-37: Excellent improvement to error handling.The updated error handling with discriminated unions provides much better debugging information compared to the previous null-checking approach. The structured error responses will help developers identify specific issues with presigned URL generation.
53-62: Consistent error handling pattern applied.The loader function correctly mirrors the same improved error handling pattern used in the action function, ensuring consistent API behavior for both PUT and GET requests.
40-40:✅ Verification successful
Verify response structure consistency.
The response structure maintains the same
presignedUrlkey, which is good for API compatibility. Ensure that client code expecting this structure continues to work correctly.Also applies to: 65-65
🏁 Script executed:
#!/bin/bash # Description: Verify that client code properly handles the presignedUrl response structure # Expected: Find usages of presignedUrl in client-side code rg -A 3 -B 3 "presignedUrl" --type ts --type tsx --type js --type jsxLength of output: 97
Let's retry the search with explicit glob patterns so ripgrep recognizes
.tsxfiles:
🏁 Script executed:
#!/bin/bash # Search for presignedUrl usages across TS, TSX, JS, and JSX files rg -g '*.{ts,tsx,js,jsx}' -A3 -B3 'presignedUrl'Length of output: 3269
No action needed:
presignedUrlremains consistentI’ve confirmed that:
- apps/webapp/app/routes/api.v1.packets.$.ts returns
{ presignedUrl: signed.url }in both branches (lines 40 and 65).- packages/core/src/v3/schemas/api.ts defines
CreateUploadPayloadUrlResponseBodywithpresignedUrl: z.string().- packages/core/src/v3/utils/ioSerialization.ts consumes
presignedResponse.presignedUrlfor both upload and download calls.All client code already expects and handles the
presignedUrlkey correctly.apps/supervisor/src/workloadManager/docker.ts (3)
31-31: Environment variable rename aligns with Docker prefix consolidation.The renaming from
RUNNER_DOCKER_NETWORKStoDOCKER_RUNNER_NETWORKSfollows the consistent Docker-prefixed environment variable pattern mentioned in the PR objectives.
64-64: OTEL endpoint moved to required variables section.Moving
OTEL_EXPORTER_OTLP_ENDPOINTto the beginning of the environment variables array aligns with its repositioning in the environment schema as a required setting.
75-76: Machine information now always available to containers.The unconditional addition of
TRIGGER_MACHINE_CPUandTRIGGER_MACHINE_MEMORYenvironment variables separates information exposure from resource enforcement, which is a good design improvement. Containers can now always access machine specifications regardless of enforcement settings.apps/webapp/app/routes/_app.orgs.$organizationSlug_.projects.new/route.tsx (2)
63-66: Simplified v3 access check removes global feature flag dependency.The removal of the global
v3Enabledfeature flag check simplifies the logic to rely solely on organization-level capabilities and managed cloud status. This aligns with the broader feature flag cleanup mentioned in the PR objectives.
126-126: Project version selection simplified to organization capability.The change from checking multiple feature flags to only checking
organization.v3Enabledstreamlines the project creation logic and removes user confusion about version selection.apps/webapp/app/env.server.ts (3)
280-281: Good addition of S3-compatible storage configuration.The new
OBJECT_STORE_REGIONandOBJECT_STORE_SERVICEenvironment variables properly support S3-compatible services with appropriate defaults.
783-789: Bootstrap environment variables are well-structured.The bootstrap configuration variables follow a consistent naming pattern and have appropriate types. The optional nature of the worker group name and token path provides flexibility.
217-218:❓ Verification inconclusive
Verify the 10x concurrency limit increase is appropriate.
The default concurrency limits have been increased from 10 to 100, which is a significant 10x increase. Ensure this change aligns with system capacity and performance requirements for v4 self-hosting environments.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any related capacity configurations or documentation that should be updated # Expected: Find related configuration files or documentation mentioning concurrency limits # Search for other concurrency-related configurations rg -i "concurrency.*limit" --type ts --type js -A 3 -B 3 # Look for performance or capacity related documentation fd -e md | xargs rg -l -i "concurrency|performance|capacity" 2>/dev/null | head -5Length of output: 67515
Verify the 10× Concurrency Limit Increase Aligns with Capacity Plans
No references to concurrency limits or performance guidance were found in the repository’s docs. Please confirm that bumping the default execution concurrency from 10 to 100 is supported by your system’s capacity and performance requirements.
• Location to review:
- apps/webapp/app/env.server.ts, lines 217–218
apps/webapp/app/bootstrap.ts (3)
8-19: Bootstrap function logic is sound.The conditional checks and error handling using
tryCatchprovide a robust bootstrap flow. The early returns for disabled bootstrap and missing worker group name are appropriate.
25-34: Good idempotency check for worker group creation.The function properly checks for existing worker groups before attempting creation, preventing duplicate entries and providing clear warning messages.
66-74: Secure token file handling implementation.The file writing implementation includes several security best practices:
- Creates parent directories recursively
- Sets restrictive file permissions (0o600)
- Provides confirmation logging
apps/webapp/app/services/platform.v3.server.ts (4)
78-91: Well-designed schema for machine preset overrides.The Zod schemas properly validate machine override values and provide type safety. The use of
MachinePresetNameenum and partial objects allows flexible overrides.
92-111: Solid initialization pattern with singleton.The machine preset initialization uses the singleton pattern appropriately and handles both override and fallback scenarios cleanly.
121-138: Correct override merging logic.The
overrideMachinesfunction properly merges override values with existing machine configurations using object spread syntax.
161-179: Robust file reading with comprehensive error handling.The
safeReadMachinePresetOverridesfunction includes proper error handling for file existence, reading, and JSON parsing with informative error logging.apps/webapp/app/v3/r2.server.ts (3)
19-23: Improved S3-compatible service configuration.The addition of
regionand conditionalserviceparameters enhances compatibility with various S3-compatible storage services. The logic for handling empty service strings is well-documented.
159-168: Excellent improvement to error handling with discriminated unions.The new return types using discriminated unions provide explicit success/failure states with detailed error messages, replacing the previous undefined returns. This is a significant improvement for error handling and debugging.
Also applies to: 215-224
169-181: Consistent error handling across both functions.Both
generatePresignedRequestandgeneratePresignedUrlnow use consistent error handling patterns with structured error responses, improving maintainability and debugging.Also applies to: 225-232
apps/supervisor/src/env.ts (4)
15-15: LGTM: OTEL endpoint moved to required settings.Moving
OTEL_EXPORTER_OTLP_ENDPOINTto the required settings section aligns with its usage pattern and makes the configuration structure more logical.
51-70: LGTM: Well-organized Docker settings consolidation.The new Docker settings section effectively consolidates Docker-related environment variables with appropriate defaults and comprehensive documentation. Key improvements:
- Clear separation of Docker-specific configuration
- Descriptive variable names with
DOCKER_prefix for consistency- Detailed documentation for
DOCKER_RUNNER_NETWORKSincluding warnings about performance implications- Sensible defaults (e.g.,
DOCKER_ENFORCE_MACHINE_PRESETS: true,DOCKER_AUTOREMOVE_EXITED_CONTAINERS: true)
76-78: LGTM: Consistent Kubernetes variable naming.The renaming of Kubernetes variables to use the
KUBERNETES_prefix improves consistency and makes the configuration more self-documenting. The variable names clearly indicate their scope and purpose.
13-13:✅ Verification successful
Verify that file:// path functionality is implemented.
The comment indicates that
TRIGGER_WORKER_TOKENaccepts afile://path to read from a file, but this schema only validates it as a string. Ensure that the actual file reading logic is implemented in the consuming code.
🏁 Script executed:
#!/bin/bash # Description: Verify that file:// path reading functionality is implemented for TRIGGER_WORKER_TOKEN # Expected: Find implementation that handles file:// prefix for reading token from file # Search for file:// handling logic in supervisor source files rg -A 10 "file://" apps/supervisor/src/ # Search for worker token file reading logic rg -A 10 "workerToken|TRIGGER_WORKER_TOKEN" apps/supervisor/src/ --type tsLength of output: 5907
TRIGGER_WORKER_TOKEN file:// support verified
The
workerToken.tsimplementation correctly handlesfile://URIs by stripping the prefix and reading the token file:• apps/supervisor/src/workerToken.ts:
- Checks for
file://prefix- Reads file via
readFileSync(tokenPath, "utf8")No changes required to the
env.tsschema.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
DEV_OTEL_EXPORTER_OTLP_ENDPOINTfallback