Add ECR support for remote builds #2224
Conversation
|
WalkthroughThe changes introduce comprehensive support for AWS Elastic Container Registry (ECR) within the deployment workflow. New optional environment variables are added to configure ECR tags and cross-account assume-role credentials. A new module implements logic to construct Docker image references for ECR, ensure ECR repositories exist, manage cross-account access using AWS STS, and retrieve ECR authentication tokens. Deployment services are updated to use these new utilities, including robust error handling and dynamic credential acquisition for ECR logins. Logging improvements are added, and new AWS SDK dependencies for ECR and STS clients are included. A dedicated test suite is added to validate ECR integration and image reference generation. ✨ 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 (
|
![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: 5
🔭 Outside diff range comments (1)
apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts (1)
82-87: Critical: Registry credential validation is incompatible with ECR.The current validation requires
DEPLOY_REGISTRY_USERNAMEandDEPLOY_REGISTRY_PASSWORDto be set, but ECR registries fetch credentials dynamically and don't need these static credentials. This will cause ECR deployments to fail.Apply this fix to make the validation conditional:
-if ( - !env.DEPLOY_REGISTRY_HOST || - !env.DEPLOY_REGISTRY_USERNAME || - !env.DEPLOY_REGISTRY_PASSWORD -) { - throw new ServiceValidationError("Missing deployment registry credentials"); -} +if (!env.DEPLOY_REGISTRY_HOST) { + throw new ServiceValidationError("Missing deployment registry host"); +} + +// For non-ECR registries, username and password are required +if (!isEcrRegistry(env.DEPLOY_REGISTRY_HOST)) { + if (!env.DEPLOY_REGISTRY_USERNAME || !env.DEPLOY_REGISTRY_PASSWORD) { + throw new ServiceValidationError("Missing deployment registry credentials"); + } +}
🧹 Nitpick comments (1)
apps/webapp/app/v3/services/initializeDeployment.server.ts (1)
72-97: Consider including the actual error in the thrown exception.While the error is logged with details, the thrown
ServiceValidationErroronly contains a generic message. Consider including the actual error for better debugging:-throw new ServiceValidationError("Failed to get deployment image ref"); +throw new ServiceValidationError(`Failed to get deployment image ref: ${imageRefError.message}`);
📜 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 (7)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/services/platform.v3.server.ts(1 hunks)apps/webapp/app/v3/getDeploymentImageRef.server.ts(1 hunks)apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts(3 hunks)apps/webapp/app/v3/services/initializeDeployment.server.ts(3 hunks)apps/webapp/package.json(1 hunks)apps/webapp/test/getDeploymentImageRef.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`apps/webapp/**/*.ts`: In the webapp, all environment variables must be accessed through the `env` export of `env.server.ts`, instead of directly accessing `process.env`.
apps/webapp/**/*.ts: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/app/services/platform.v3.server.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/services/finalizeDeploymentV2.server.tsapps/webapp/app/env.server.tsapps/webapp/test/getDeploymentImageRef.test.tsapps/webapp/app/v3/getDeploymentImageRef.server.ts
`apps/webapp/**/*.{ts,tsx}`: When importing from `@trigger.dev/core` in the weba...
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/app/services/platform.v3.server.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/services/finalizeDeploymentV2.server.tsapps/webapp/app/env.server.tsapps/webapp/test/getDeploymentImageRef.test.tsapps/webapp/app/v3/getDeploymentImageRef.server.ts
`**/*.{ts,tsx}`: Always prefer using isomorphic code like fetch, ReadableStream,...
**/*.{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
Use strict mode
No default exports, use function declarations
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
apps/webapp/app/services/platform.v3.server.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/services/finalizeDeploymentV2.server.tsapps/webapp/app/env.server.tsapps/webapp/test/getDeploymentImageRef.test.tsapps/webapp/app/v3/getDeploymentImageRef.server.ts
`{packages/core,apps/webapp}/**/*.{ts,tsx}`: We use zod a lot in packages/core and in the webapp
{packages/core,apps/webapp}/**/*.{ts,tsx}: We use zod a lot in packages/core and in the webapp
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
apps/webapp/app/services/platform.v3.server.tsapps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/services/finalizeDeploymentV2.server.tsapps/webapp/app/env.server.tsapps/webapp/test/getDeploymentImageRef.test.tsapps/webapp/app/v3/getDeploymentImageRef.server.ts
`apps/webapp/**/*.test.{ts,tsx}`: The `env.server.ts` file should never be imported into a test file, either directly or indirectly.
apps/webapp/**/*.test.{ts,tsx}: Theenv.server.tsfile should never be imported into a test file, either directly or indirectly.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/test/getDeploymentImageRef.test.ts
`**/*.test.{ts,tsx}`: Our tests are all vitest
**/*.test.{ts,tsx}: Our tests are all vitest
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
apps/webapp/test/getDeploymentImageRef.test.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: .github/workflows/release-helm.yml:42-46
Timestamp: 2025-06-25T13:24:23.836Z
Learning: In .github/workflows/release-helm.yml, the user nicktrn confirmed that using 'entrypoint' with 'docker://' steps works fine, contrary to previous analysis suggesting it's unsupported.
apps/webapp/app/services/platform.v3.server.ts (2)
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#1608
File: apps/webapp/app/v3/services/triggerTask.server.ts:418-418
Timestamp: 2025-01-13T18:31:48.160Z
Learning: The `MachinePresetName` schema is used to validate machine preset values in the trigger.dev codebase, ensuring type safety and validation of machine preset options.
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2150
File: apps/supervisor/src/workloadManager/docker.ts:115-115
Timestamp: 2025-06-04T16:02:22.957Z
Learning: In the Trigger.dev codebase, the supervisor component uses DOCKER_ENFORCE_MACHINE_PRESETS while the docker provider component uses ENFORCE_MACHINE_PRESETS. These are separate components with separate environment variable configurations for the same logical concept of enforcing machine presets.
apps/webapp/app/v3/services/initializeDeployment.server.ts (8)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When triggering a task from backend code, use `tasks.trigger`, `tasks.batchTrigger`, or `tasks.triggerAndPoll` from `@trigger.dev/sdk/v3` and set the `TRIGGER_SECRET_KEY` environment variable.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.494Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : You MUST use `@trigger.dev/sdk/v3` when writing Trigger.dev tasks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Before generating any code for Trigger.dev tasks, verify that you are importing from `@trigger.dev/sdk/v3`, exporting every task, and not generating any deprecated code patterns.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.494Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : ALWAYS generate Trigger.dev tasks using the `task` function from `@trigger.dev/sdk/v3` and NEVER use the deprecated `client.defineJob` pattern.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.494Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : You MUST NEVER use `client.defineJob` in Trigger.dev task files, as it is deprecated and will break the application.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Applies to trigger.config.ts : The `trigger.config.ts` file must use `defineConfig` from `@trigger.dev/sdk/v3` to configure project settings, directories, retries, telemetry, runtime, machine settings, log level, max duration, and build options.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Schema tasks must use `schemaTask` from `@trigger.dev/sdk/v3` and validate payloads using a schema (e.g., Zod).
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When implementing idempotency, use the `idempotencyKeys` API from `@trigger.dev/sdk/v3` and provide an `idempotencyKey` when triggering tasks.
apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts (2)
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#1367
File: apps/coordinator/src/checkpointer.ts:588-631
Timestamp: 2024-10-12T01:08:24.066Z
Learning: Errors in the `#createDockerCheckpoint` method are handled in the caller, so additional error logging within this method is unnecessary.
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#1367
File: apps/coordinator/src/checkpointer.ts:588-631
Timestamp: 2024-09-30T11:35:37.554Z
Learning: Errors in the `#createDockerCheckpoint` method are handled in the caller, so additional error logging within this method is unnecessary.
apps/webapp/app/env.server.ts (2)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.413Z
Learning: Applies to apps/webapp/**/*.ts : In the webapp, all environment variables must be accessed through the `env` export of `env.server.ts`, instead of directly accessing `process.env`.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.413Z
Learning: Applies to apps/webapp/app/**/*.test.{ts,tsx} : Tests should only import classes and functions from files matching `app/**/*.ts` of the webapp, and those files should not use environment variables; everything should be passed through as options instead.
apps/webapp/test/getDeploymentImageRef.test.ts (3)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.413Z
Learning: Applies to apps/webapp/app/**/*.test.{ts,tsx} : Tests should only import classes and functions from files matching `app/**/*.ts` of the webapp, and those files should not use environment variables; everything should be passed through as options instead.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:21:33.965Z
Learning: Applies to **/*.test.{ts,tsx} : Our tests are all vitest
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.413Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : The `env.server.ts` file should never be imported into a test file, either directly or indirectly.
🧬 Code Graph Analysis (3)
apps/webapp/app/v3/services/initializeDeployment.server.ts (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
getDeploymentImageRef(97-149)
apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts (3)
packages/cli-v3/src/utilities/fileSystem.ts (1)
createTempDir(92-100)apps/webapp/app/v3/getDeploymentImageRef.server.ts (2)
isEcrRegistry(151-153)getEcrAuthToken(307-341)apps/webapp/app/env.server.ts (1)
env(890-890)
apps/webapp/test/getDeploymentImageRef.test.ts (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (4)
parseEcrRegistryDomain(241-259)createEcrClient(78-95)getDeploymentImageRef(97-149)getEcrAuthToken(307-341)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 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 (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
apps/webapp/package.json (1)
36-38: LGTM!The AWS SDK dependencies for ECR and STS are correctly added to support the new ECR functionality.
apps/webapp/app/services/platform.v3.server.ts (1)
105-106: Good addition for observability.The logging statement helps track when machine presets are overridden, which is useful for debugging and monitoring.
apps/webapp/app/env.server.ts (1)
232-241: Environment variables properly configured for ECR support.The new optional environment variables follow the existing pattern and provide the necessary configuration for ECR repository tagging and cross-account access via AWS STS AssumeRole.
apps/webapp/app/v3/services/initializeDeployment.server.ts (1)
11-12: Good refactoring to centralize image reference logic.The changes properly extract the image reference generation to a dedicated function and improve error handling.
apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts (3)
12-13: Good additions for ECR support.The imports properly support the new ECR authentication functionality.
177-197: Excellent error handling improvement.The use of
tryCatchprovides better error handling and logging for registry login failures.
270-282: Well-implemented ECR authentication logic.The code correctly:
- Detects ECR registries
- Fetches fresh credentials using the configured assume role
- Validates that auth is provided for non-ECR registries
apps/webapp/test/getDeploymentImageRef.test.ts (2)
25-43: LGTM! Well-structured cleanup with proper error handling.The afterAll hook correctly handles repository cleanup with appropriate error handling to prevent test failures from cleanup issues.
123-139: Excellent validation of ECR auth token format.The test thoroughly validates the auth token structure including the base64 format check, which ensures the token is properly formatted.
apps/webapp/app/v3/getDeploymentImageRef.server.ts (4)
162-195: Excellent security defaults for ECR repository creation.The function correctly sets up ECR repositories with:
- Immutable tags to prevent tag overwrites
- AES256 encryption for security
- Proper error handling and logging
33-49: Well-designed session management for AssumeRole.The implementation includes:
- Unique session names for debugging
- Clear documentation about AWS limits
- Appropriate 1-hour duration for ECR operations
241-259: Robust ECR domain validation.The function correctly validates all components of an ECR registry domain, ensuring it matches the expected format exactly.
297-302: Good defensive validation of repository creation.Verifying that the created repository name matches the requested name is excellent defensive programming, even though AWS should always return the correct name.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/webapp/test/getDeploymentImageRef.test.ts (4)
11-24: Remove direct environment variable access in tests.The test file directly accesses
process.envvariables, which violates coding guidelines for webapp tests. Tests should receive all configuration as parameters rather than reading environment variables directly.This issue was already flagged in previous reviews. Please refactor to pass configuration values as explicit parameters to the functions under test.
28-28: Environment variable usage in test cleanup violates guidelines.The
process.env.KEEP_TEST_REPOcheck should be removed. Test cleanup should be controlled through test configuration parameters, not environment variables.- if (process.env.KEEP_TEST_REPO === "1") { + if (testConfig.keepTestRepo) { return; }
108-124: Test dependency on previous test execution order.This test depends on a repository created by the previous test, creating fragile test dependencies. Tests should be independent and not rely on execution order.
Consider creating the repository explicitly within this test or using a
beforeEachhook to ensure consistent test state.
141-150: Duplicate environment variable access violation.More direct
process.envaccess in the second test suite. This continues the same violation pattern from the first test suite.Apply the same refactoring approach to remove all
process.envreferences and pass configuration as parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/v3/getDeploymentImageRef.server.ts(1 hunks)apps/webapp/app/v3/services/initializeDeployment.server.ts(3 hunks)apps/webapp/test/getDeploymentImageRef.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/v3/services/initializeDeployment.server.ts
🧰 Additional context used
📓 Path-based instructions (6)
`apps/webapp/**/*.ts`: In the webapp, all environment variables must be accessed through the `env` export of `env.server.ts`, instead of directly accessing `process.env`.
apps/webapp/**/*.ts: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/test/getDeploymentImageRef.test.tsapps/webapp/app/v3/getDeploymentImageRef.server.ts
`apps/webapp/**/*.test.{ts,tsx}`: The `env.server.ts` file should never be imported into a test file, either directly or indirectly.
apps/webapp/**/*.test.{ts,tsx}: Theenv.server.tsfile should never be imported into a test file, either directly or indirectly.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/test/getDeploymentImageRef.test.ts
`apps/webapp/**/*.{ts,tsx}`: When importing from `@trigger.dev/core` in the weba...
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/test/getDeploymentImageRef.test.tsapps/webapp/app/v3/getDeploymentImageRef.server.ts
`**/*.{ts,tsx}`: Always prefer using isomorphic code like fetch, ReadableStream,...
**/*.{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
Use strict mode
No default exports, use function declarations
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
apps/webapp/test/getDeploymentImageRef.test.tsapps/webapp/app/v3/getDeploymentImageRef.server.ts
`**/*.test.{ts,tsx}`: Our tests are all vitest
**/*.test.{ts,tsx}: Our tests are all vitest
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
apps/webapp/test/getDeploymentImageRef.test.ts
`{packages/core,apps/webapp}/**/*.{ts,tsx}`: We use zod a lot in packages/core and in the webapp
{packages/core,apps/webapp}/**/*.{ts,tsx}: We use zod a lot in packages/core and in the webapp
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
apps/webapp/test/getDeploymentImageRef.test.tsapps/webapp/app/v3/getDeploymentImageRef.server.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: .github/workflows/release-helm.yml:42-46
Timestamp: 2025-06-25T13:24:23.836Z
Learning: In .github/workflows/release-helm.yml, the user nicktrn confirmed that using 'entrypoint' with 'docker://' steps works fine, contrary to previous analysis suggesting it's unsupported.
apps/webapp/test/getDeploymentImageRef.test.ts (10)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.413Z
Learning: Applies to apps/webapp/app/**/*.test.{ts,tsx} : Tests should only import classes and functions from files matching `app/**/*.ts` of the webapp, and those files should not use environment variables; everything should be passed through as options instead.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:21:33.965Z
Learning: Applies to **/*.test.{ts,tsx} : Our tests are all vitest
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.413Z
Learning: Applies to apps/webapp/**/*.test.{ts,tsx} : The `env.server.ts` file should never be imported into a test file, either directly or indirectly.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-06-24T08:07:11.818Z
Learning: When writing tests in the monorepo, always follow the procedures and standards outlined in the 'tests.md' guide located at 'mdc:ai/references/tests.md'.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/executing-commands.mdc:0-0
Timestamp: 2025-06-30T13:21:44.520Z
Learning: When running tests, it is often better to `cd` into the package directory and then run tests using `pnpm run test ...`.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.413Z
Learning: Applies to apps/webapp/**/*.ts : In the webapp, all environment variables must be accessed through the `env` export of `env.server.ts`, instead of directly accessing `process.env`.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: If you generate code that fails the verification steps (import, export, deprecated patterns), your implementation will break in production, fail to deploy, and fail to run locally.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Before generating any code for Trigger.dev tasks, verify that you are importing from `@trigger.dev/sdk/v3`, exporting every task, and not generating any deprecated code patterns.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.413Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : When importing from `@trigger.dev/core` in the webapp, never import from the root `@trigger.dev/core` path; always use one of the subpath exports as defined in the package's package.json.
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2155
File: hosting/docker/.env.example:4-7
Timestamp: 2025-06-06T23:55:01.933Z
Learning: In the trigger.dev project, .env.example files should contain actual example secret values rather than placeholders, as these help users understand the expected format. The files include clear warnings about not using these defaults in production and instructions for generating proper secrets.
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2155
File: docs/docs.json:179-183
Timestamp: 2025-06-06T16:54:23.316Z
Learning: In the docs.json configuration for the Trigger.dev documentation (Mintlify system), both "tags": ["v4"] and "tag": "v4" properties can be used together and work correctly, even though this behavior is undocumented and may not work in local development environments.
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (8)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (7)
20-76: Well-implemented assume role functionality.The
getAssumedRoleCredentialsfunction properly handles STS assume role operations with good error handling, session naming, and credential validation. The implementation correctly manages temporary credentials and provides detailed logging for debugging.
78-95: Clean ECR client factory implementation.The
createEcrClientfunction provides a clean abstraction for creating ECR clients with optional assume role support. The conditional logic is clear and the implementation is straightforward.
97-152: Robust main function with proper error handling.The
getDeploymentImageReffunction is well-structured with proper error handling usingtryCatchand comprehensive logging. The function correctly handles both ECR and non-ECR registries and returns appropriate metadata.
154-161: ECR registry validation properly addresses past feedback.The
isEcrRegistryfunction now usesparseEcrRegistryDomainfor more specific validation, which addresses the previous security concern about broadamazonaws.commatching.
163-192: Tag parsing function addresses previous validation concerns.The
parseRegistryTagsfunction now includes proper input validation, handles edge cases like empty tags and missing equals signs, and provides appropriate logging for invalid tags. This addresses the previous feedback about input validation.
273-291: Precise ECR domain parsing with good validation.The
parseEcrRegistryDomainfunction provides robust validation of ECR registry hostnames with clear error messages. The parsing logic correctly handles the expected 6-part domain structure.
345-379: Secure ECR authentication token handling.The
getEcrAuthTokenfunction properly handles ECR authentication token retrieval with appropriate error checking and Base64 decoding. The implementation correctly extracts username and password from the authorization token.apps/webapp/test/getDeploymentImageRef.test.ts (1)
180-244: Well-structured unit tests for utility functions.The unit tests for
parseEcrRegistryDomainandparseRegistryTagsare well-designed with good coverage of edge cases, error conditions, and various input formats. These tests don't have environment variable dependencies and are properly isolated.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
11-11: Update import to use subpath export for tryCatch.The import from
@trigger.dev/coreviolates the coding guideline requiring webapp imports to use subpath exports instead of root imports.-import { tryCatch } from "@trigger.dev/core"; +import { tryCatch } from "@trigger.dev/core/utils";
🧹 Nitpick comments (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
355-358: Remove redundant region validation check.The
regioncheck is unnecessary sinceparseEcrRegistryDomainwould have already thrown an error if the registry host was invalid, ensuringregionis always defined at this point.- if (!region) { - logger.error("Invalid ECR registry host", { registryHost }); - throw new Error("Invalid ECR registry host"); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`apps/webapp/**/*.ts`: In the webapp, all environment variables must be accessed through the `env` export of `env.server.ts`, instead of directly accessing `process.env`.
apps/webapp/**/*.ts: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
`apps/webapp/**/*.{ts,tsx}`: When importing from `@trigger.dev/core` in the weba...
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
`**/*.{ts,tsx}`: Always prefer using isomorphic code like fetch, ReadableStream,...
**/*.{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
Use strict mode
No default exports, use function declarations
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
`{packages/core,apps/webapp}/**/*.{ts,tsx}`: We use zod a lot in packages/core and in the webapp
{packages/core,apps/webapp}/**/*.{ts,tsx}: We use zod a lot in packages/core and in the webapp
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2195
File: .github/workflows/release-helm.yml:42-46
Timestamp: 2025-06-25T13:24:23.836Z
Learning: In .github/workflows/release-helm.yml, the user nicktrn confirmed that using 'entrypoint' with 'docker://' steps works fine, contrary to previous analysis suggesting it's unsupported.
apps/webapp/app/v3/getDeploymentImageRef.server.ts (13)
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#2155
File: docs/docs.json:179-183
Timestamp: 2025-06-06T16:54:23.316Z
Learning: In the docs.json configuration for the Trigger.dev documentation (Mintlify system), both "tags": ["v4"] and "tag": "v4" properties can be used together and work correctly, even though this behavior is undocumented and may not work in local development environments.
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#1306
File: .github/actions/get-image-tag/action.yml:51-62
Timestamp: 2024-09-23T12:51:42.019Z
Learning: In the 'get-image-tag' GitHub Action, prefer dependent workflows to fail immediately when the tag is invalid, without outputting the validity status as an output.
Learnt from: nicktrn
PR: triggerdotdev/trigger.dev#1306
File: .github/actions/get-image-tag/action.yml:51-62
Timestamp: 2024-10-12T01:08:24.066Z
Learning: In the 'get-image-tag' GitHub Action, prefer dependent workflows to fail immediately when the tag is invalid, without outputting the validity status as an output.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.413Z
Learning: Applies to apps/webapp/**/*.{ts,tsx} : When importing from `@trigger.dev/core` in the webapp, never import from the root `@trigger.dev/core` path; always use one of the subpath exports as defined in the package's package.json.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Before generating any code for Trigger.dev tasks, verify that you are importing from `@trigger.dev/sdk/v3`, exporting every task, and not generating any deprecated code patterns.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.494Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : You MUST `export` every task, including subtasks, in Trigger.dev task files.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.494Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : You MUST use `@trigger.dev/sdk/v3` when writing Trigger.dev tasks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.494Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : ALWAYS generate Trigger.dev tasks using the `task` function from `@trigger.dev/sdk/v3` and NEVER use the deprecated `client.defineJob` pattern.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Schema tasks must use `schemaTask` from `@trigger.dev/sdk/v3` and validate payloads using a schema (e.g., Zod).
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.413Z
Learning: Applies to apps/webapp/app/**/*.test.{ts,tsx} : Tests should only import classes and functions from files matching `app/**/*.ts` of the webapp, and those files should not use environment variables; everything should be passed through as options instead.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `runs` and `tasks` APIs from `@trigger.dev/sdk/v3` to subscribe to run updates and implement realtime features.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : The `run` function must contain the main logic for each Trigger.dev task.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Each Trigger.dev task must have a unique `id` within your project.
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- 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 / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 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 / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- 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: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
1-381: LGTM! Well-structured ECR integration module.The implementation provides comprehensive ECR support with proper error handling, logging, and AWS SDK integration. Previous review feedback has been successfully addressed including ECR registry validation and tag parsing improvements.
Summary:
DEPLOY_REGISTRY_HOSTdetectionNew (optional) env vars:
DEPLOY_REGISTRY_ECR_TAGSDEPLOY_REGISTRY_ECR_ASSUME_ROLE_ARNDEPLOY_REGISTRY_ECR_ASSUME_ROLE_EXTERNAL_ID