Skip to content

Conversation

aq17
Copy link

@aq17 aq17 commented Feb 3, 2022

Previously, the organizational example did not create a cloud account and include the management account in the benchmark task scope

@wideawakening wideawakening changed the title Option to create management cloud account feat: enable benchmark on organizational management account Feb 3, 2022
variable "provision_in_management_account" {
type = bool
default = true
description = "Whether to deploy the stack in the management account"
Copy link
Contributor

Choose a reason for hiding this comment

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

guess we can hold exposing this variable to the examples

Copy link
Author

Choose a reason for hiding this comment

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

👍🏼

@wideawakening
Copy link
Contributor

wideawakening commented Feb 3, 2022

doing some maintenance on module so will add the "cft stackset instance" to managed account on diagrams too

Copy link
Contributor

@nkraemer-sysdig nkraemer-sysdig left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @aq17!

@wideawakening
Copy link
Contributor

i was iterating this yesterday and got 409 cloud-account setup conflict over and over again thinking it may be due to other causes, but after properly passing checks on other PR's (with some cleanup).. i'm thinking there may be something wrong with these changes.

been digging on the organizations_organization data and seems like you got it all right, but dunno what's making it fail. could
you give a look see if you spot what's going on?
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/organizations_organization#master-account-attributes-reference


some non-critical code review

  • per terraform-guideliness, there should only be one local block, positioned on the top of the manifest
  • account_ids_to_deploy seems like concat(local.member_account_ids, [data.aws_organizations_organization.org[0].master_account_id])

@aq17
Copy link
Author

aq17 commented Feb 4, 2022

i was iterating this yesterday and got 409 cloud-account setup conflict over and over again thinking it may be due to other causes, but after properly passing checks on other PR's (with some cleanup).. i'm thinking there may be something wrong with these changes.

been digging on the organizations_organization data and seems like you got it all right, but dunno what's making it fail. could you give a look see if you spot what's going on? https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/organizations_organization#master-account-attributes-reference

some non-critical code review

  • per terraform-guideliness, there should only be one local block, positioned on the top of the manifest
  • account_ids_to_deploy seems like concat(local.member_account_ids, [data.aws_organizations_organization.org[0].master_account_id])

Is the 409 for all accounts or specifically the master account?

@wideawakening
Copy link
Contributor

fix for tests through the provider running smoothly
thx for the work @aq17!

@wideawakening wideawakening merged commit e86cfd8 into master Feb 8, 2022
@wideawakening wideawakening deleted the SSPROD-10941 branch February 8, 2022 10:26
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.

3 participants