Skip to content

Add support for organization aggregator #85

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Feb 7, 2024

Conversation

IslamHeggy
Copy link
Contributor

@IslamHeggy IslamHeggy commented Feb 6, 2024

what

  • Extended the module functionality to include organization wide aggregator
  • Add the ability to create/pass new IAM role for the organization aggregator
  • Handled default IAM role cases vs organization aggregator IAM role. So they don't depend on each other

why

  • The current default way is attaching accounts using account ids and there is no way to use organization wide aggregator and it's really hard to maintain large number of accounts when using organizations.

references

  • I used organization aggregation argument of the aws_config_configuration_aggregator provider to add the functionality.

  • I checked this stale PR and decided to reinvent the wheel as it has been a while since it was opened

@IslamHeggy IslamHeggy requested review from a team as code owners February 6, 2024 02:42
@IslamHeggy IslamHeggy force-pushed the feat/add_organization_aggregator branch from c11c769 to 94c714b Compare February 6, 2024 02:49
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IslamHeggy thanks for the PR

Please address the error

[tflint] reported by reviewdog 🐶
data "aws_iam_policy_document" "config_organization_aggregator_policy" is declared but not used

and run the following commands and commit the changes

make init
make github/init
make readme

Thanks

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see comments

@IslamHeggy
Copy link
Contributor Author

please see comments

I addressed all your comments in my recent commit, please let me know if anything else is needed here @aknysh

@IslamHeggy IslamHeggy requested a review from aknysh February 7, 2024 04:20
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
@IslamHeggy IslamHeggy requested a review from aknysh February 7, 2024 15:19
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
@IslamHeggy IslamHeggy requested a review from aknysh February 7, 2024 15:42
IslamHeggy and others added 2 commits February 7, 2024 22:24
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
@aknysh
Copy link
Member

aknysh commented Feb 7, 2024

@IslamHeggy sorry, please run again

make init
make github/init
make readme

@IslamHeggy
Copy link
Contributor Author

@IslamHeggy sorry, please run again

make init
make github/init
make readme

That's okay, I updated the documentation @aknysh

@aknysh
Copy link
Member

aknysh commented Feb 7, 2024

/terratest

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @IslamHeggy

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.

2 participants