-
-
Notifications
You must be signed in to change notification settings - Fork 952
chore(deployments): lazily update ECR repo cache settings #2734
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
To avoid doing a migration for the ECR repo cache settings, we lazily do it on the next deployment for that project. Failures to update the repo settings are just logged and will not cause the deployment to fail.
|
WalkthroughThis change introduces ECR lifecycle management and cross-account access capabilities to the deployment image reference module. It adds a new lifecycle policy to expire untagged images older than 3 days, implements tag mutability settings (IMMUTABLE_WITH_EXCLUSION with a cache tag exception), and introduces cross-account ECR repository access via IAM role assumption. The repository creation and management logic is updated to enforce these policies automatically upon creation and when updating existing repositories. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
268-315: Consider partial failure scenario.If
PutImageTagMutabilityCommandsucceeds butPutLifecyclePolicyCommandfails, the repository will have the new mutability setting but lack the lifecycle policy for cleaning untagged images. This partial state means old cache images won't be expired automatically.Since failures are logged and don't fail deployments (per PR objective), the next deployment attempt would retry the update. However, the condition at line 407 checks for
imageTagMutability === "IMMUTABLE", so a partially updated repo (nowIMMUTABLE_WITH_EXCLUSION) won't trigger a retry of the lifecycle policy.Consider wrapping the lifecycle policy update separately or adjusting the retry condition to also check for missing lifecycle policy.
One potential approach:
async function updateEcrRepositoryCacheSettings({ repositoryName, region, accountId, assumeRole, }: { repositoryName: string; region: string; accountId?: string; assumeRole?: AssumeRoleConfig; }): Promise<void> { logger.debug("Updating ECR repository tag mutability to IMMUTABLE_WITH_EXCLUSION", { repositoryName, region, }); const ecr = await createEcrClient({ region, assumeRole }); - await ecr.send( + const [mutabilityError] = await tryCatch(ecr.send( new PutImageTagMutabilityCommand({ repositoryName, registryId: accountId, imageTagMutability: "IMMUTABLE_WITH_EXCLUSION", imageTagMutabilityExclusionFilters: [ { // only the `cache` tag will be mutable, all other tags will be immutable filter: "cache", filterType: "WILDCARD", }, ], }) - ); + )); + + if (mutabilityError) { + logger.error("Failed to update ECR repository tag mutability", { + repositoryName, + region, + error: mutabilityError, + }); + throw mutabilityError; + } // When the `cache` tag is mutated, the old cache images are untagged. // This policy matches those images and expires them to avoid bloating the repository. await ecr.send( new PutLifecyclePolicyCommand({ repositoryName, registryId: accountId, lifecyclePolicyText: untaggedImageExpirationPolicy, }) ); logger.debug("Successfully updated ECR repository to IMMUTABLE_WITH_EXCLUSION", { repositoryName, region, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
🔇 Additional comments (5)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (5)
10-10: LGTM!Import is correctly added and used in the
updateEcrRepositoryCacheSettingsfunction.
200-214: LGTM!Good refactoring to extract the lifecycle policy into a reusable constant. The policy structure follows the AWS ECR lifecycle policy format correctly.
257-263: LGTM!Correctly uses the extracted
untaggedImageExpirationPolicyconstant.
231-248: LGTM!The repository creation correctly applies
IMMUTABLE_WITH_EXCLUSIONwith the cache tag exception, ensuring new repositories are created with the desired settings from the start.
405-419: The lazy update logic correctly targets repos needing migration.The code checks
imageTagMutability === "IMMUTABLE"to identify repos that need updating (migrating toIMMUTABLE_WITH_EXCLUSION), while skipping already-migrated repos.IMMUTABLE_WITH_EXCLUSIONis a valid AWS ECR value. Error handling aligns with the PR objective of not failing deployments on update errors.
To avoid doing a migration for the ECR repo cache settings, we lazily do it on the next deployment for that project. Failures to update the repo settings are just logged and will not cause the deployment to fail.