Update ECR/Docker naming conventions and enable cost anomaly detection#825
Update ECR/Docker naming conventions and enable cost anomaly detection#825forstmeier merged 2 commits intomasterfrom
Conversation
github-actions
Bot
added
python
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 53 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR reorganizes Docker image namespacing and ECR repository naming across the infrastructure. It renames the Tide model-runner to "runner", restructures ECR repository names to distinguish between "applications" and "models" namespaces, updates corresponding Pulumi exports, and introduces namespace-based build logic in the build script. Additionally, cost anomaly detection resources are added to monitor AWS spending. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR renames ECR repositories and Dockerfile stages to a namespace-prefixed convention ( Confidence Score: 4/5Safe to merge after fixing the stale ECR repo names in devenv.nix; all infrastructure changes are internally consistent. One P1 finding: devenv.nix retains the old un-prefixed ECR repository names (fund/data-manager-server etc.) while storage.py now provisions fund/applications-data-manager-server etc. The ecr-push devenv script uses these values directly to build image URIs, so local image pushes will fail with a repository-not-found error. All other changed files are consistently updated. devenv.nix — ecrRepo values at lines 11, 16, 21 need the applications- prefix added
|
| Filename | Overview |
|---|---|
| devenv.nix | ECR repo names in ecrRepo fields were not updated to match the new applications- prefixed naming convention; ecr-push will fail at runtime |
| infrastructure/notifications.py | Adds cost anomaly monitor and subscription with correct CE expression syntax (plural Dimensions key); prior review concerns addressed |
| infrastructure/storage.py | ECR repository names updated to namespace-prefixed convention and lifecycle policy extended with git-tagged image retention rule |
| infrastructure/main.py | Pulumi export names updated consistently to match new ECR naming convention |
| maskfile.md | build-and-push command updated with namespace-based repo naming; GHA cache scope updated to include namespace prefix |
| .github/workflows/launch_infrastructure.yaml | Tide build matrix stage renamed from model-runner to runner, consistent with Dockerfile stage rename |
| models/tide/Dockerfile | Final build stage renamed from model-runner to runner |
| libraries/python/tests/test_infrastructure_notifications.py | New test file covering cost anomaly monitor, subscription, and budget resources following existing string-based test pattern |
| libraries/python/tests/test_infrastructure_storage.py | Updated lifecycle policy assertion from tide_model_runner_repository_lifecycle to tide_runner_repository_lifecycle |
| models/tide/tests/test_deploy.py | Image URI in test updated from fund/tide-model-runner to fund/models-tide-runner |
Comments Outside Diff (1)
-
devenv.nix, line 11-21 (link)Stale ECR repository names not updated
The
ecrRepovalues here still use the old repository names without theapplications-namespace prefix. Theecr-pushscript at line 156 uses these values to build the full image URI and push to ECR, so runningecr-pushafter this PR merges will attempt to push tofund/data-manager-server,fund/ensemble-manager-server, andfund/portfolio-manager-server— repositories that no longer exist. The actual repos are now namedfund/applications-data-manager-server, etc.Prompt To Fix With AI
This is a comment left during a code review. Path: devenv.nix Line: 11-21 Comment: **Stale ECR repository names not updated** The `ecrRepo` values here still use the old repository names without the `applications-` namespace prefix. The `ecr-push` script at line 156 uses these values to build the full image URI and push to ECR, so running `ecr-push` after this PR merges will attempt to push to `fund/data-manager-server`, `fund/ensemble-manager-server`, and `fund/portfolio-manager-server` — repositories that no longer exist. The actual repos are now named `fund/applications-data-manager-server`, etc. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: devenv.nix
Line: 11-21
Comment:
**Stale ECR repository names not updated**
The `ecrRepo` values here still use the old repository names without the `applications-` namespace prefix. The `ecr-push` script at line 156 uses these values to build the full image URI and push to ECR, so running `ecr-push` after this PR merges will attempt to push to `fund/data-manager-server`, `fund/ensemble-manager-server`, and `fund/portfolio-manager-server` — repositories that no longer exist. The actual repos are now named `fund/applications-data-manager-server`, etc.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "Address PR #825 feedback: fix CE Express..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR standardizes Docker/ECR naming across models and applications and extends the Pulumi infrastructure stack to include AWS Cost Explorer anomaly detection notifications.
Changes:
- Renames the Tide model Docker build stage/tagging from
model-runnertorunnerand updates related references (tests, workflow matrix, Mask tasks). - Updates ECR repository naming to include a
models-/applications-namespace prefix and renames corresponding Pulumi variables/exports. - Adds Cost Explorer anomaly monitor + subscription resources to the infrastructure notifications module.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| models/tide/tests/test_deploy.py | Updates expected ECR image URI to the new models-tide-runner repository name. |
| models/tide/Dockerfile | Renames the final build stage to runner to match the new convention. |
| maskfile.md | Updates image build/push logic to add models/applications namespace and updates Pulumi output key used for Tide deploys. |
| libraries/python/tests/test_infrastructure_storage.py | Updates assertions for renamed ECR lifecycle policy resource IDs. |
| infrastructure/training.py | Switches ECS task definition image reference to the renamed Tide image URI output. |
| infrastructure/storage.py | Adds lifecycle rule for tagged images and renames ECR repositories/resources for new naming convention. |
| infrastructure/notifications.py | Adds Cost Explorer anomaly monitor/subscription resources for cost anomaly detection. |
| infrastructure/main.py | Renames Pulumi exports for ECR image/repository outputs to match new naming convention. |
| .github/workflows/launch_infrastructure.yaml | Updates Tide build target stage from model-runner to runner. |
Comments suppressed due to low confidence (1)
maskfile.md:88
- The
repository_namenow targets new ECR repositories (e.g.fund/models-tide-runner,fund/applications-*-server). In CI, the workflow runs the build/push job beforepulumi up, so these repositories may not exist yet on the first run after this rename anddocker buildx --pushwill fail. Ensure the repository exists before pushing (e.g., create it if missing in this script, or reorder the workflow so Pulumi provisions ECR before the build/push step).
repository_name="fund/${namespace}-${package_name}-${stage_name}"
image_reference="${aws_account_id}.dkr.ecr.${aws_region}.amazonaws.com/${repository_name}"
echo "Logging into ECR"
aws ecr get-login-password --region ${aws_region} | docker login \
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
coverage: 77.462% (+0.2%) from 77.252% — ecr-and-docker-naming-updates into master |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/launch_infrastructure.yaml:
- Around line 31-33: Update both paths-filter blocks so naming-only changes
trigger image rebuilds: add this workflow filename, maskfile.md, and
infrastructure/storage.py to the include list in the existing paths-filter that
controls the image jobs, and mirror the same additions in the deploy_images
job's paths-filter block so renames (e.g., the stage/application naming changes
such as "application: tide") cause the image jobs to run.
In `@infrastructure/notifications.py`:
- Around line 50-57: The threshold_expression JSON uses the wrong key
"Dimension" which causes Pulumi/AWS to reject the AnomalySubscription; update
the JSON under threshold_expression to use the plural key "Dimensions" instead
of "Dimension" and keep the same inner structure (Key, Values, MatchOptions) so
the AnomalySubscription creation succeeds; locate the threshold_expression
assignment in the code (the json.dumps block) and change the key to
"Dimensions".
In `@infrastructure/storage.py`:
- Around line 122-123: The ECR rename (changing repository name
"fund/applications-data-manager-server") must be performed as a staged migration
rather than an in-place change: first add and provision the new ECR resource(s)
(so Pulumi creates the new physical repositories) and backfill them by running
build_and_push_images to push images to the new repo(s); then update
producers/consumers to use the new repo names and add Pulumi aliases on the ECR
resource(s) in the infrastructure/storage.py resource declaration to preserve
state continuity during rename; only after consumers are switched and images are
verified, remove the old repository resources (avoiding force_delete
replacements that would delete existing images). Run pulumi preview --diff
against production before merging to confirm the replacement semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22ef75ce-4d95-4b07-85f8-b364cd905e0b
📒 Files selected for processing (9)
.github/workflows/launch_infrastructure.yamlinfrastructure/__main__.pyinfrastructure/notifications.pyinfrastructure/storage.pyinfrastructure/training.pylibraries/python/tests/test_infrastructure_storage.pymaskfile.mdmodels/tide/Dockerfilemodels/tide/tests/test_deploy.py
…tests Fix "Dimension" (singular) to "Dimensions" (plural) in the AnomalySubscription threshold_expression. The AWS Cost Explorer Expression API requires the plural form; the singular key would cause the subscription to fail provisioning or fire on every anomaly regardless of dollar impact. Add test_infrastructure_notifications.py covering the cost anomaly monitor, subscription, and budget resources, following the same string-based pattern used by other infrastructure test modules. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Overview
Changes
Context
Some random stuff.
Summary by CodeRabbit
New Features
Chores