Conversation
Pull Request Review Status
Working Directory: |
Pull Request Review Status
Working Directory: |
gambol99
force-pushed
the
feat/rework_module
branch
3 times, most recently
from
394e264 to
8adf6fb
Compare
There was a problem hiding this comment.
Pull request overview
This PR restructures the Terraform modules for Appvia support roles (notably the LZA module), updates AWS provider requirements, and refreshes documentation/examples to match the new module layout and outputs.
Changes:
- Refactors
modules/lzato use dedicateddata.tf/locals.tf, consolidates cost-analysis permissions onto the primary support role, and removes the separate cost-analysis role/output. - Bumps AWS provider constraints (LZA module + examples to
>= 6.0.0) and updates example lockfiles accordingly. - Expands/rewrites READMEs to better explain resources, trust model, and usage.
Reviewed changes
Copilot reviewed 21 out of 25 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/lza/variables.tf | Updates defaults (external account, cost-analysis enabled by default), simplifies inputs, adds default tags map |
| modules/lza/terraform.tf | Bumps required AWS provider to >= 6.0.0 |
| modules/lza/policies.tf | Removes old locals lists and adjusts comments/formatting |
| modules/lza/outputs.tf | Removes cost-analysis role output; keeps landing support role ARN output |
| modules/lza/main.tf | Refactors role trust policy to use aws_iam_policy_document, changes policy/attachment structure |
| modules/lza/locals.tf | Introduces computed locals for ARNs and managed policy list |
| modules/lza/data.tf | Adds aws_caller_identity and trust policy document datasource |
| modules/lza/README.md | Rewrites module documentation and updates terraform-docs injected sections |
| modules/costanalysis/variables.tf | Updates external account + role name defaults |
| modules/costanalysis/terraform.tf | Adds module-level Terraform/provider requirements |
| modules/costanalysis/policies.tf | Removes header comments |
| modules/costanalysis/outputs.tf | Removes header comments |
| modules/costanalysis/main.tf | Moves locals out; minor comment cleanup |
| modules/costanalysis/locals.tf | Adds locals file for role name and external role ARN |
| modules/costanalysis/README.md | Trims generated sections; updates defaults in inputs table |
| modules/costanalysis/.terraform.lock.hcl | Removes module lockfile |
| examples/lza-support/terraform.tf | Bumps example AWS provider constraint to >= 6.0.0 |
| examples/lza-support/outputs.tf | Removes cost-analysis output from the example |
| examples/lza-support/README.md | Adds narrative description for what the example provisions |
| examples/lza-support/.terraform.lock.hcl | Updates lockfile to AWS provider v6 |
| examples/cost-analysis-support/terraform.tf | Bumps example AWS provider constraint to >= 6.0.0 |
| examples/cost-analysis-support/README.md | Expands narrative documentation and updates tfvars filename reference |
| examples/cost-analysis-support/.terraform.lock.hcl | Updates lockfile, resulting in redundant constraint string |
| README.md | Adds module index and links to module/example docs |
| .github/workflows/terraform.yml | Adds YAML doc marker and removes vnext PR trigger branch |
Files not reviewed (3)
- examples/cost-analysis-support/.terraform.lock.hcl: Language not supported
- examples/lza-support/.terraform.lock.hcl: Language not supported
- modules/costanalysis/.terraform.lock.hcl: Language not supported
Comments suppressed due to low confidence (3)
modules/lza/terraform.tf:8
- Provider constraint is bumped to
>= 6.0.0, which is a major-version upgrade from the previous v4.x constraint and can be a breaking change for consumers. If v6 is required, consider tightening this to~> 6.0(or documenting the minimum tested version) and call out the breaking change in module docs/release notes.
modules/lza/variables.tf:16 - The description for
enable_cost_analysis_supportstill says it “creates the finops role”, but this module no longer creates a separate cost-analysis role—only an additional policy/attachments. Please update the variable description to reflect the current behavior to avoid confusing module users.
variable "external_region" {
description = "The external region from where the support role will be assumed"
type = string
default = "eu-west-2"
modules/lza/variables.tf:17
- Changing
enable_cost_analysis_supportdefault totruemakes the module grant additional billing/cost permissions by default (and attach extra managed policies). Consider keeping the defaultfalseto follow least-privilege, and require explicit opt-in from consumers; if intentional, document this as a behavioral/breaking change.
variable "external_region" {
description = "The external region from where the support role will be assumed"
type = string
default = "eu-west-2"
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
This PR refactors the Terraform support-role modules (LZA + Cost Analysis) to simplify role/policy wiring, update defaults/docs, and bump AWS provider requirements in the LZA module and examples.
Changes:
- Refactors
modules/lzato use a generated trust policy document, consolidates attachments, and removes the separate cost-analysis role/output in favor of attaching an optional cost-analysis policy to the LZA support role. - Adds
locals.tf/data.tfstructure and updates defaults (external account/role, tags default, cost analysis enabled by default). - Updates documentation and examples; bumps AWS provider constraints (notably to
>= 6.0.0for LZA + examples) and adjusts CI workflow triggers.
Reviewed changes
Copilot reviewed 21 out of 25 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/lza/main.tf | Refactors IAM role/policy resources and attachments; removes separate cost-analysis role wiring. |
| modules/lza/data.tf | Adds aws_iam_policy_document for the LZA role trust policy. |
| modules/lza/locals.tf | Centralizes role name, external role ARN, and managed policy ARNs. |
| modules/lza/variables.tf | Updates defaults (external account/role), makes tags optional, and changes cost-analysis default. |
| modules/lza/policies.tf | Minor comment/whitespace edits around policy documents. |
| modules/lza/outputs.tf | Removes cost-analysis output; retains LZA role output. |
| modules/lza/terraform.tf | Bumps AWS provider constraint to >= 6.0.0. |
| modules/lza/README.md | Rewrites module documentation and updates injected terraform-docs block. |
| modules/costanalysis/terraform.tf | Adds module-level Terraform/provider requirements. |
| modules/costanalysis/locals.tf | Extracts locals previously in main.tf. |
| modules/costanalysis/variables.tf | Updates defaults (external account/role). |
| modules/costanalysis/main.tf | Removes inline locals block and cleans up comments. |
| modules/costanalysis/policies.tf | Removes header comments only. |
| modules/costanalysis/outputs.tf | Removes header comments only. |
| modules/costanalysis/README.md | Expanded documentation and updated terraform-docs injection. |
| modules/costanalysis/.terraform.lock.hcl | Removes module lockfile. |
| examples/lza-support/terraform.tf | Bumps AWS provider constraint to >= 6.0.0. |
| examples/lza-support/outputs.tf | Removes cost-analysis output from example. |
| examples/lza-support/README.md | Expanded example documentation and updates terraform-docs section. |
| examples/lza-support/.terraform.lock.hcl | Updates provider lock to AWS 6.36.0. |
| examples/cost-analysis-support/terraform.tf | Bumps AWS provider constraint to >= 6.0.0. |
| examples/cost-analysis-support/README.md | Expanded example documentation and updates terraform-docs section. |
| examples/cost-analysis-support/.terraform.lock.hcl | Updates provider lock; constraints now include redundant combined bounds. |
| README.md | Adds module overview sections and links to module/example READMEs. |
| .github/workflows/terraform.yml | Adds YAML document header and removes vnext PR branch trigger. |
Files not reviewed (3)
- examples/cost-analysis-support/.terraform.lock.hcl: Language not supported
- examples/lza-support/.terraform.lock.hcl: Language not supported
- modules/costanalysis/.terraform.lock.hcl: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gambol99
force-pushed
the
feat/rework_module
branch
from
8adf6fb to
ef2dff7
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the Terraform “support roles” modules by restructuring the LZA module (moving trust policy + locals out, simplifying attachments) and improving module/example documentation, while also bumping the AWS provider baseline for the LZA module and its examples.
Changes:
- Refactors
modules/lzato use a dedicated trust policy data source + locals, and attaches cost-analysis permissions to the main LZA support role instead of creating a separate role/output. - Updates defaults (external account/role,
enable_cost_analysis_support,tags) and bumps AWS provider constraints (notably LZA to>= 6.0.0). - Reworks READMEs and example docs to be more descriptive and aligns example/provider lockfiles with the new constraints.
Reviewed changes
Copilot reviewed 21 out of 25 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/lza/variables.tf | Updates defaults (external account/role, enablement) and makes tags default to {}. |
| modules/lza/terraform.tf | Bumps AWS provider constraint to >= 6.0.0. |
| modules/lza/policies.tf | Removes old locals header; keeps policy documents; minor comment formatting. |
| modules/lza/outputs.tf | Removes cost-analysis output; retains LZA role output. |
| modules/lza/main.tf | Refactors role trust policy + policy/attachment resources; changes cost-analysis handling to policy attachment. |
| modules/lza/locals.tf | Introduces new locals for role name, external role ARN, and managed policy list. |
| modules/lza/data.tf | Adds IAM trust policy document data source for the LZA support role. |
| modules/lza/README.md | Rewrites module README and updates terraform-docs provider/version section. |
| modules/costanalysis/variables.tf | Updates default external account/role name values. |
| modules/costanalysis/terraform.tf | Adds terraform/provider requirements for the module. |
| modules/costanalysis/policies.tf | Removes old header comments only. |
| modules/costanalysis/outputs.tf | Removes old header comments only. |
| modules/costanalysis/main.tf | Moves locals out and cleans up comments. |
| modules/costanalysis/locals.tf | Adds locals for role name and external role ARN. |
| modules/costanalysis/README.md | Rewrites module README with detailed permissions and usage. |
| modules/costanalysis/.terraform.lock.hcl | Removes the module lockfile. |
| examples/lza-support/terraform.tf | Bumps example AWS provider constraint to >= 6.0.0. |
| examples/lza-support/outputs.tf | Removes the example’s cost-analysis output. |
| examples/lza-support/README.md | Expands narrative docs; updates tfvars filename reference. |
| examples/lza-support/.terraform.lock.hcl | Updates lockfile to AWS provider v6. |
| examples/cost-analysis-support/terraform.tf | Bumps example AWS provider constraint to >= 6.0.0. |
| examples/cost-analysis-support/README.md | Expands narrative docs; updates tfvars filename reference. |
| examples/cost-analysis-support/.terraform.lock.hcl | Updates lockfile to AWS provider v6. |
| README.md | Adds module catalog + links and expands repository overview. |
| .github/workflows/terraform.yml | Adds YAML doc start and removes vnext from PR trigger branches. |
Files not reviewed (3)
- examples/cost-analysis-support/.terraform.lock.hcl: Language not supported
- examples/lza-support/.terraform.lock.hcl: Language not supported
- modules/costanalysis/.terraform.lock.hcl: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gambol99
force-pushed
the
feat/rework_module
branch
2 times, most recently
from
ff3d3ae to
e18d883
Compare
There was a problem hiding this comment.
Pull request overview
This PR reworks the Terraform support-role modules (LZA + Cost Analysis), updating IAM role/policy structure, provider constraints, and accompanying documentation/examples.
Changes:
- Refactors the
modules/lzamodule to use a dedicated trust policy document and shared role attachments (rather than separate roles/locals embedded in multiple files). - Updates default variable values and removes the LZA cost-analysis role/output in favor of attaching optional cost-analysis policies to the primary support role.
- Bumps AWS provider constraints (notably
modules/lzaand examples) and refreshes README content.
Reviewed changes
Copilot reviewed 21 out of 25 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/lza/variables.tf | Updates defaults (account/role, enable_cost_analysis_support) and makes tags optional. |
| modules/lza/terraform.tf | Raises AWS provider requirement to >= 6.0.0. |
| modules/lza/policies.tf | Removes unused locals header and minor comment formatting. |
| modules/lza/outputs.tf | Updates output description and removes cost-analysis output from LZA module. |
| modules/lza/main.tf | Refactors role creation and policy attachments; fixes/updates descriptions/comments. |
| modules/lza/locals.tf | Introduces locals for role name, external role ARN, and managed policy ARNs (currently has a syntax issue). |
| modules/lza/data.tf | Adds aws_iam_policy_document for the role trust policy. |
| modules/lza/README.md | Adds richer module documentation + updates terraform-docs injected section. |
| modules/costanalysis/variables.tf | Updates defaults for external account/role name. |
| modules/costanalysis/terraform.tf | Adds explicit Terraform + provider requirements for the module. |
| modules/costanalysis/policies.tf | Removes header comments only. |
| modules/costanalysis/outputs.tf | Removes header comments only. |
| modules/costanalysis/main.tf | Moves locals out; minor comment cleanup. |
| modules/costanalysis/locals.tf | Adds locals previously in main.tf. |
| modules/costanalysis/README.md | Adds richer module documentation + terraform-docs injected section. |
| modules/costanalysis/.terraform.lock.hcl | Removes committed lockfile from module directory. |
| examples/lza-support/terraform.tf | Raises AWS provider constraint to >= 6.0.0. |
| examples/lza-support/outputs.tf | Removes cost-analysis output exposure from the example. |
| examples/lza-support/README.md | Expands narrative docs; terraform-docs injected section remains (but appears inconsistent). |
| examples/lza-support/.terraform.lock.hcl | Updates lockfile to AWS provider v6. |
| examples/cost-analysis-support/terraform.tf | Raises AWS provider constraint to >= 6.0.0. |
| examples/cost-analysis-support/README.md | Expands narrative docs; terraform-docs injected section remains (but appears inconsistent). |
| examples/cost-analysis-support/.terraform.lock.hcl | Updates lockfile to AWS provider v6 (constraint string now redundant). |
| README.md | Expands root-level docs to describe modules and link examples. |
| .github/workflows/terraform.yml | Removes vnext PR trigger branch and adds YAML document marker. |
Files not reviewed (3)
- examples/cost-analysis-support/.terraform.lock.hcl: Language not supported
- examples/lza-support/.terraform.lock.hcl: Language not supported
- modules/costanalysis/.terraform.lock.hcl: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # | ||
| ## Provision the IAM policy for custom cusdos permissions | ||
| # | ||
| ## Provision the IAM policy for custom cusdos permissions |
| version = "5.39.1" | ||
| constraints = ">= 5.0.0, ~> 5.0" | ||
| version = "6.36.0" | ||
| constraints = ">= 5.0.0, >= 6.0.0" |
Comment on lines
19
to
23
| variable "enable_cost_analysis_support" { | ||
| description = "Enable the creation of the finops role in the customer account" | ||
| type = bool | ||
| default = false | ||
| default = true | ||
| } |
|
|
||
| ## Providers | ||
|
|
||
| No providers. |
| description = "The name of the IAM role to be assumed by the support team" | ||
| description = "The ARN of the IAM role to be assumed by the support team" | ||
| value = aws_iam_role.lza_support_role.arn | ||
| } |
|
|
||
| ## Providers | ||
|
|
||
| No providers. |
Comment on lines
+12
to
+16
| var.enable_cost_analysis_support ? [ | ||
| "arn:aws:iam::aws:policy/AWSBillingConductorReadOnlyAccess", | ||
| "arn:aws:iam::aws:policy/AWSBillingReadOnlyAccess", | ||
| "arn:aws:iam::aws:policy/CostOptimizationHubReadOnlyAccess", | ||
| ] : []) |
| } | ||
|
|
||
| variable "external_lza_role_name" { | ||
| description = "The external account name from where the support role will be assumed" |
| # | ||
| ## Provision if required the cost analysis policy | ||
| # | ||
| ## Privision the IAM policy to support cost analysis if enabled |
| @@ -2,8 +2,3 @@ output "appvia_landing_zone_support_role_arn" { | |||
| description = "The name of the Landing Zone Support IAM role to be assumed by the Appvia support team" | |||
gambol99
force-pushed
the
feat/rework_module
branch
2 times, most recently
from
9e49f95 to
1a7914c
Compare
gambol99
force-pushed
the
feat/rework_module
branch
from
1a7914c to
b00c434
Compare
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
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.
No description provided.