fix(recipes): document aws-efa regional ECR override pattern#774
Merged
Conversation
Closes #764. Non-breaking: us-west-2 stays as the in-tree default; this PR adds the documentation and override paths needed for non-us-west-2 deployments via the existing --set / --dynamic mechanisms. What's added: - recipes/components/aws-efa/values.yaml: comment block above the image.repository field explaining the regional ECR pattern, the override paths (--set at bundle time, --dynamic at install time), and the partition-aware account IDs (standard / GovCloud / China). - docs/integrator/recipe-development.md: new "Regional registry overrides" section under Advanced Topics with worked --set and --dynamic examples and the partition table. - docs/user/container-images.md (prose): the "Regional ECR" entry in the Registries spanned section now flags that the us-west-2 default shown in the auto-generated table is overridable, with a link to the recipe-development doc. What's NOT changed: - The default image URI in values.yaml stays at us-west-2. Existing bundles produced without an override behave identically — no behavior change for current users. - AICR's bundler does not (today) process Go templates on base values files, so the fix uses the existing --set / --dynamic mechanisms rather than introducing a new templating layer in pkg/bundler. Adding template processing would be a feature change with broader scope than this issue calls for. The published BOM auto-generated section is therefore byte-identical to before; only the prose changes. Verified via md5 round-trip on make bom-docs.
Contributor
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
Coverage Report ✅
Coverage BadgeNo Go source files changed in this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/integrator/recipe-development.md (1)
70-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the Table of Contents to include the new section.
The new
### Regional registry overridessection (Line 491) is missing from the Advanced Topics ToC block, which makes navigation inconsistent in this long guide.As per coding guidelines: “When writing documentation… clearly structure content… and document operational clarity.”Suggested edit
- [Advanced Topics](`#advanced-topics`) - [External Data Sources](`#external-data-sources`) + - [Regional registry overrides](`#regional-registry-overrides`)Also applies to: 491-491
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/integrator/recipe-development.md` around lines 70 - 72, The Advanced Topics table-of-contents block is missing the new "Regional registry overrides" section; update the ToC by adding an entry that matches the heading text and slug (e.g., add a bullet like "Regional registry overrides" linking to "#regional-registry-overrides") under the "Advanced Topics" list so the new section at heading "Regional registry overrides" is discoverable in the document; ensure the link text exactly matches the H3 heading and the anchor follows the repo's slug rules used elsewhere in the ToC.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@recipes/components/aws-efa/values.yaml`:
- Line 23: The comment points out a wording mismatch in the values.yaml comment
referencing "across all standard AWS partitions"; update the comment that
mentions "Account ID 602401143452" to say the account ID is stable across
standard AWS regions (or simply "AWS regions") instead of "partitions" so it
correctly reflects regional stability and avoids confusion with the partition
table below.
---
Outside diff comments:
In `@docs/integrator/recipe-development.md`:
- Around line 70-72: The Advanced Topics table-of-contents block is missing the
new "Regional registry overrides" section; update the ToC by adding an entry
that matches the heading text and slug (e.g., add a bullet like "Regional
registry overrides" linking to "#regional-registry-overrides") under the
"Advanced Topics" list so the new section at heading "Regional registry
overrides" is discoverable in the document; ensure the link text exactly matches
the H3 heading and the anchor follows the repo's slug rules used elsewhere in
the ToC.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5d197a84-73d2-4d31-b0f2-2e6db075c055
📒 Files selected for processing (3)
docs/integrator/recipe-development.mddocs/user/container-images.mdrecipes/components/aws-efa/values.yaml
| # to pull from. The default below points at us-west-2; deploying in any other | ||
| # region requires overriding the registry's region segment. | ||
| # | ||
| # Account ID 602401143452 is stable across all standard AWS partitions. |
There was a problem hiding this comment.
Clarify partition wording to avoid a factual mismatch.
Line 23 says “across all standard AWS partitions,” but there is only one standard partition (aws); the stability is across standard AWS regions. Recommend rewording to avoid confusion with the partition table below.
Suggested edit
-# Account ID 602401143452 is stable across all standard AWS partitions.
+# Account ID 602401143452 is stable across standard AWS regions (partition `aws`).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Account ID 602401143452 is stable across all standard AWS partitions. | |
| # Account ID 602401143452 is stable across standard AWS regions (partition `aws`). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@recipes/components/aws-efa/values.yaml` at line 23, The comment points out a
wording mismatch in the values.yaml comment referencing "across all standard AWS
partitions"; update the comment that mentions "Account ID 602401143452" to say
the account ID is stable across standard AWS regions (or simply "AWS regions")
instead of "partitions" so it correctly reflects regional stability and avoids
confusion with the partition table below.
lalitadithya
approved these changes
May 6, 2026
This was referenced May 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #764. Non-breaking:
us-west-2stays as the in-tree default; this PR adds the documentation and override paths needed for non-us-west-2 deployments via AICR's existing--set/--dynamicmechanisms.Motivation / Context
recipes/components/aws-efa/values.yamlpins the EFA device plugin image to602401143452.dkr.ecr.us-west-2.amazonaws.com/eks/aws-efa-k8s-device-plugin. AWS publishes EKS add-ons regionally — every EKS cluster pulls from its own region's private ECR over the AWS internal backbone. Customers in any region other than us-west-2 today either pay cross-region NAT egress to pull from us-west-2 or fail entirely if cross-region access is blocked.Fixes #764. Refs #739.
Type of Change
Component(s) Affected
recipes/components/aws-efa/values.yaml— comment only)docs/integrator/recipe-development.md,docs/user/container-images.md)Implementation Notes
Why documentation rather than templating. AICR's bundler doesn't process Go templates on base values files today — the override mechanisms are
--set(bake-in at bundle time) and--dynamic(placeholder for install-time fill). The original issue's draft fix proposed{{ .region }}syntax invalues.yaml; verifying the bundler showed that wouldn't actually work. Adding template processing on base values is a feature change with broader scope. The pragmatic fix uses the existing mechanisms.Comment block in values.yaml explains the regional pattern and shows both override paths plus partition-aware account IDs (standard / GovCloud / China).
Recipe-development doc gets a new Regional registry overrides section under Advanced Topics with worked examples for both
--set(single-region bundle) and--dynamic(one bundle, many regions) plus the partition table.BOM doc prose flags the regional caveat in the Registries spanned section so a reader can immediately tell the
us-west-2host shown in the auto-generated table is overridable.Auto-generated BOM table is byte-identical because in-tree defaults didn't change. Verified via md5 round-trip on
make bom-docs.Testing
Risk Assessment
Rollout notes: Existing bundles produced without an override behave identically. New deployments outside us-west-2 should follow the new doc section.
Checklist
make testwith-race)make lint)git commit -S)