refactor: remove policy files and simplify providers#61
Conversation
Remove policy-TerraformUpdateAccess.tf and policy-Identity-role-TeamAccess.tf files. Simplify providers.tf to basic provider configuration with dummy iam_roles module. Update main.tf to remove root account assignment handling and associated permission sets. Remove unused variables from variables.tf. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughRemoves root-specific account assignment branching and module, consolidates account_assignments, extends account_map schema, simplifies provider/iam_roles wiring, removes several public variables, adds mixin-based policy files, and adds a v2.0.0 migration guide and changelog. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
aws-identity-center/src/main.tf
Lines 94 to 99 in b51ca8b
Preserve root account assignment handling
All account assignments are now applied through the default provider (sso_account_assignments uses local.account_assignments directly). When account_assignments includes the management/root account and the component is deployed from a delegated admin/identity account, AWS Identity Center APIs reject those root assignments (they require management account credentials). The previous split for root vs non-root avoided this; with the split removed, root assignments will fail in that scenario.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/providers.tf`:
- Around line 3-7: The change to variable "account_map_enabled" flips its
default from true to false which is a breaking behavioral change for existing
users; either revert the default back to true in the variable declaration
(restore previous behavior for variable "account_map_enabled") or, if the new
default is intentional, explicitly document the breaking change in the PR
description and release notes and add a migration guide (and consider a version
bump) so consumers know to opt-in to the new behavior.
- Around line 33-37: The dummy module "iam_roles" uses cloudposse/label/null
which doesn't export the expected outputs (global_tenant_name,
global_environment_name, global_stage_name) referenced by the remote-state
usage; fix by either swapping the "iam_roles" module to one that exposes those
outputs or adapt the remote-state configuration to use the existing context
outputs (tenant, environment, stage) from module. Locate the module "iam_roles"
declaration and either replace its source with a module that defines
global_tenant_name/global_environment_name/global_stage_name, or add a
mapping/local values so remote-state references the available outputs (e.g., map
module.iam_roles.context.tenant -> global_tenant_name) and update remote-state
references accordingly.
…scription - Move policy-TerraformUpdateAccess.tf and policy-Identity-role-TeamAccess.tf to mixins folder - Restore account_map_enabled variable description and default to true Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add CHANGELOG.md entry for v2.0.0 breaking changes - Create MIGRATION.md with step-by-step upgrade instructions - Document static account_map support for bypassing remote state - Update mixins README with documentation for all available mixins Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Important Do not edit the Please update the Could you fix it @Benbentwo? 🙏 |
- Add migration section with link to MIGRATION.md - Document static account map support - Update examples to use account_map_enabled: false - Update mixins list with new mixins - Update identity_roles_accessible section to reference mixin Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
CI was failing because the iam_roles dummy module was missing a version pin. Added version = "0.25.0" to match the version used in context.tf. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Auto-generated documentation update reflecting v2.0.0 changes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
milldr
left a comment
milldr
left a comment
There was a problem hiding this comment.
Please remove all references to account-map in the new version. For example, remote state.
Document tf_access_bucket_arn and tf_access_role_arn variables required when using the TerraformUpdateAccess mixin. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Line 510: The README module table is out of sync: it shows
cloudposse/label/null as "n/a" while src/providers.tf pins version = "0.25.0";
regenerate the docs from README.yaml (e.g., run `atmos docs` or your
repository's docs generation command) so README.md reflects the pinned provider
version and commit the updated README.md.
🧹 Nitpick comments (3)
src/providers.tf (1)
1-2: Comment text is inconsistent with default behavior.The comment states "when account map is disabled" but the default for
account_map_enabledistrue(enabled). This could confuse users reading the code.Consider updating the comment to accurately reflect the file's purpose:
📝 Suggested fix
-# This is the default providers.tf when account map is disabled. +# This is the default providers.tf for the aws-identity-center component. +# Set account_map_enabled to false to use static account mappings instead of remote state lookups.src/MIGRATION.md (1)
79-88: Add language specifier to fenced code block.The error message code block is missing a language specifier. Per markdownlint (MD040), fenced code blocks should have a language specified for consistency and proper rendering.
📝 Suggested fix
-``` +```text Error: Provider configuration not present To work with module.sso_account_assignments_root.aws_ssoadmin_account_assignment.this[...] (orphan) its original provider configuration at provider["registry.terraform.io/hashicorp/aws"].root is required, but it has been removed.</details> </blockquote></details> <details> <summary>README.md (1)</summary><blockquote> `160-164`: **Add language specifier to fenced code block.** The code block showing the Terraform format expression is missing a language specifier. Per markdownlint (MD040), add `hcl` for proper syntax highlighting. <details> <summary>📝 Suggested fix</summary> ```diff -``` +```hcl format("Identity%sTeamAccess", replace(title(replace(team, "_", "-")), "-", ""))</details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Update module version from n/a to 0.25.0 to match src/providers.tf. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 161-163: The fenced code block showing
format("Identity%sTeamAccess", replace(title(replace(team, "_", "-")), "-", ""))
is missing a language tag in the generated README.md; open the source
README.yaml, locate the corresponding code block entry and add the appropriate
language identifier (e.g., bash, text, or the template language you use) to the
fence, then regenerate README.md from README.yaml using the atmos docs commands
so the change is preserved (do not edit README.md directly).
Add language identifier to fenced code block showing the Terraform format function for permission set names. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
These changes were released in v2.0.0. |
2 participants
Summary
Removes two policy definition files (TerraformUpdateAccess and Identity-role-TeamAccess) and simplifies the providers configuration. The component now uses basic AWS provider setup with a dummy IAM roles module.
Changes
policy-TerraformUpdateAccess.tfandpolicy-Identity-role-TeamAccess.tfproviders.tfto remove complex role assumption logicmain.tfto remove root account assignment handlingvariables.tfTesting
Terraform validate passed successfully.
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
New Features
Chores