Skip to content

Conversation

@myftija
Copy link
Collaborator

@myftija myftija commented Dec 3, 2025

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.

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.
@changeset-bot
Copy link

changeset-bot bot commented Dec 3, 2025

⚠️ No Changeset found

Latest commit: 0e29236

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2025

Walkthrough

This 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

  • getAssumedRoleCredentials implementation: Verify IAM role assumption logic, credential handling, and error scenarios for cross-account access
  • updateEcrRepositoryCacheSettings function: Confirm tag mutability configuration, exclusion rules for cache tags, and lifecycle policy application
  • ensureEcrRepositoryExists updates: Review the logic for detecting immutable repositories and triggering cache settings updates, including error handling and logging
  • PutImageTagMutabilityCommand usage: Validate correct API usage and parameter passing for enforcing tag immutability rules
  • Security considerations: Assess credential scope, lifecycle, and proper cleanup for assumed-role credentials

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description is sparse and missing required template sections like testing, changelog, and checklist items required by the repository template. Complete the PR description by adding testing steps, changelog entry, the contribution checklist, and issue reference to match the repository template.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: lazily updating ECR repo cache settings during deployments instead of via migration.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ecr-repo-lazy-update

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 PutImageTagMutabilityCommand succeeds but PutLifecyclePolicyCommand fails, 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 (now IMMUTABLE_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

📥 Commits

Reviewing files that changed from the base of the PR and between 255a73a and 0e29236.

📒 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 env export of env.server.ts instead of directly accessing process.env in 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/core in 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 updateEcrRepositoryCacheSettings function.


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 untaggedImageExpirationPolicy constant.


231-248: LGTM!

The repository creation correctly applies IMMUTABLE_WITH_EXCLUSION with 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 to IMMUTABLE_WITH_EXCLUSION), while skipping already-migrated repos. IMMUTABLE_WITH_EXCLUSION is a valid AWS ECR value. Error handling aligns with the PR objective of not failing deployments on update errors.

@myftija myftija merged commit 341e27d into main Dec 3, 2025
31 checks passed
@myftija myftija deleted the ecr-repo-lazy-update branch December 3, 2025 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants