Skip to content
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

Add support for additional deployment_targets config for aws_cloudformation_stack_set_instance #26935

Conversation

hhughes0
Copy link

@hhughes0 hhughes0 commented Sep 22, 2022

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Release Note:

  • resource/aws_cloudformation_stack_set_instance: Extend deployment_targets argument

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccCloudFormationStackSet_\|TestAccCloudFormationStackSetInstance_' PKG=cloudformation ACCTEST_PARALLELISM=3 TF_ACC_LOG_PATH=/Users/harrison.hughes/repos/terraform-provider-aws/logs/log3.txt TF_LOG_CORE
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/cloudformation/... -v -count 1 -parallel 3  -run=TestAccCloudFormationStackSet_\|TestAccCloudFormationStackSetInstance_ -timeout 180m
=== RUN   TestAccCloudFormationStackSetInstance_basic
=== PAUSE TestAccCloudFormationStackSetInstance_basic
=== RUN   TestAccCloudFormationStackSetInstance_disappears
=== PAUSE TestAccCloudFormationStackSetInstance_disappears
=== RUN   TestAccCloudFormationStackSetInstance_Disappears_stackSet
=== PAUSE TestAccCloudFormationStackSetInstance_Disappears_stackSet
=== RUN   TestAccCloudFormationStackSetInstance_parameterOverrides
=== PAUSE TestAccCloudFormationStackSetInstance_parameterOverrides
=== RUN   TestAccCloudFormationStackSetInstance_retainStack
=== PAUSE TestAccCloudFormationStackSetInstance_retainStack
=== RUN   TestAccCloudFormationStackSetInstance_deploymentTargets
=== PAUSE TestAccCloudFormationStackSetInstance_deploymentTargets
=== RUN   TestAccCloudFormationStackSetInstance_operationPreferences
=== PAUSE TestAccCloudFormationStackSetInstance_operationPreferences
=== RUN   TestAccCloudFormationStackSet_basic
=== PAUSE TestAccCloudFormationStackSet_basic
=== RUN   TestAccCloudFormationStackSet_disappears
=== PAUSE TestAccCloudFormationStackSet_disappears
=== RUN   TestAccCloudFormationStackSet_administrationRoleARN
=== PAUSE TestAccCloudFormationStackSet_administrationRoleARN
=== RUN   TestAccCloudFormationStackSet_description
=== PAUSE TestAccCloudFormationStackSet_description
=== RUN   TestAccCloudFormationStackSet_executionRoleName
=== PAUSE TestAccCloudFormationStackSet_executionRoleName
=== RUN   TestAccCloudFormationStackSet_name
=== PAUSE TestAccCloudFormationStackSet_name
=== RUN   TestAccCloudFormationStackSet_operationPreferences
=== PAUSE TestAccCloudFormationStackSet_operationPreferences
=== RUN   TestAccCloudFormationStackSet_parameters
=== PAUSE TestAccCloudFormationStackSet_parameters
=== RUN   TestAccCloudFormationStackSet_Parameters_default
    acctest.go:69: this resource does not currently ignore unconfigured CloudFormation template parameters with the Default property
--- SKIP: TestAccCloudFormationStackSet_Parameters_default (0.00s)
=== RUN   TestAccCloudFormationStackSet_Parameters_noEcho
    acctest.go:69: this resource does not currently ignore CloudFormation template parameters with the NoEcho property
--- SKIP: TestAccCloudFormationStackSet_Parameters_noEcho (0.00s)
=== RUN   TestAccCloudFormationStackSet_PermissionModel_serviceManaged
    acctest.go:69: API does not support enabling Organizations access (in particular, creating the Stack Sets IAM Service-Linked Role)
--- SKIP: TestAccCloudFormationStackSet_PermissionModel_serviceManaged (0.00s)
=== RUN   TestAccCloudFormationStackSet_tags
=== PAUSE TestAccCloudFormationStackSet_tags
=== RUN   TestAccCloudFormationStackSet_templateBody
=== PAUSE TestAccCloudFormationStackSet_templateBody
=== RUN   TestAccCloudFormationStackSet_templateURL
=== PAUSE TestAccCloudFormationStackSet_templateURL
=== CONT  TestAccCloudFormationStackSetInstance_basic
=== CONT  TestAccCloudFormationStackSet_administrationRoleARN
=== CONT  TestAccCloudFormationStackSet_parameters
--- PASS: TestAccCloudFormationStackSet_administrationRoleARN (46.63s)
=== CONT  TestAccCloudFormationStackSetInstance_deploymentTargets
--- PASS: TestAccCloudFormationStackSet_parameters (85.57s)
=== CONT  TestAccCloudFormationStackSet_disappears
--- PASS: TestAccCloudFormationStackSetInstance_basic (96.40s)
=== CONT  TestAccCloudFormationStackSet_basic
--- PASS: TestAccCloudFormationStackSet_disappears (18.00s)
=== CONT  TestAccCloudFormationStackSetInstance_operationPreferences
--- PASS: TestAccCloudFormationStackSet_basic (24.31s)
=== CONT  TestAccCloudFormationStackSetInstance_parameterOverrides
--- PASS: TestAccCloudFormationStackSetInstance_deploymentTargets (129.52s)
=== CONT  TestAccCloudFormationStackSetInstance_retainStack
--- PASS: TestAccCloudFormationStackSetInstance_parameterOverrides (190.13s)
=== CONT  TestAccCloudFormationStackSet_name
--- PASS: TestAccCloudFormationStackSetInstance_retainStack (160.94s)
=== CONT  TestAccCloudFormationStackSet_operationPreferences
--- PASS: TestAccCloudFormationStackSet_name (42.72s)
=== CONT  TestAccCloudFormationStackSetInstance_Disappears_stackSet
--- PASS: TestAccCloudFormationStackSetInstance_Disappears_stackSet (82.46s)
=== CONT  TestAccCloudFormationStackSetInstance_disappears
--- PASS: TestAccCloudFormationStackSet_operationPreferences (125.08s)
=== CONT  TestAccCloudFormationStackSet_templateBody
--- PASS: TestAccCloudFormationStackSet_templateBody (43.17s)
=== CONT  TestAccCloudFormationStackSet_templateURL
--- PASS: TestAccCloudFormationStackSetInstance_disappears (109.16s)
=== CONT  TestAccCloudFormationStackSet_executionRoleName
--- PASS: TestAccCloudFormationStackSet_templateURL (44.86s)
=== CONT  TestAccCloudFormationStackSet_description
--- PASS: TestAccCloudFormationStackSet_description (42.97s)
=== CONT  TestAccCloudFormationStackSet_tags
--- PASS: TestAccCloudFormationStackSet_executionRoleName (57.46s)
--- PASS: TestAccCloudFormationStackSetInstance_operationPreferences (514.53s)
--- PASS: TestAccCloudFormationStackSet_tags (108.40s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/cloudformation	703.273s

...

Closes

Closes #26917

Other Notes

I've mostly adapted the work done by @jnixon-blue here #23908

We have a use case where it would be great to have this support added to target multiple accounts within an organization with the use of SERVICE_MANAGED permissions. Ive tested these changes to the provider locally and they work as expected.

@github-actions
Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

  • Please review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. service/cloudformation Issues and PRs that pertain to the cloudformation service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. needs-triage Waiting for first response or review from a maintainer. size/S Managed by automation to categorize the size of a PR. labels Sep 22, 2022
@hhughes0 hhughes0 changed the title Add support for additional deploymen_targets config for stacksetinstance Add support for additional deployment_targets config for stacksetinstance Sep 22, 2022
@hhughes0 hhughes0 changed the title Add support for additional deployment_targets config for stacksetinstance Add support for additional deploymen_targets config for aws_cloudformation_stack_set_instance Sep 22, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome @hhughes0 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTOR guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

@github-actions github-actions bot added size/M Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Sep 22, 2022
@hhughes0
Copy link
Author

hhughes0 commented Sep 22, 2022

@ewbankkit I noticed you reviewed the PR I linked in the description, would you be able to cast your eyes over this one too by any chance?

Acceptance tests are now passing.

@hhughes0 hhughes0 changed the title Add support for additional deploymen_targets config for aws_cloudformation_stack_set_instance Add support for additional deployment_targets config for aws_cloudformation_stack_set_instance Sep 23, 2022
@breathingdust breathingdust added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Oct 11, 2022
@mdrdannyr
Copy link

@hhughes0

Hey hope you're doing well.

Your PR has helped a lot with some internal projects. I would love to see this merged into main.

However, in our usage we're seeing something a bit strange.

Creation works like a charm. But if i add another account_id to the accounts list in the stackset instance, it doesn't actually do anything. Similar if i delete the account from the list.

as an example:

Running this change:

deployment_targets {
    organizational_unit_ids = ["r-1abc"]
    account_filter_type = "INTERSECTION"
    accounts = ["12345678910"]
  }

to

deployment_targets {
    organizational_unit_ids = ["r-1abc"]
    account_filter_type = "INTERSECTION"
    accounts = ["12345678910", "12345678911"]
  }

Doesn't actually deploy another stack instance to the new account ("12345678911"). Similar occurs if we remove the account id from the list.

I'm looking at the code but not 100% sure where to start here regarding debugging.

Is this something you've experienced as well?

@sosheskaz
Copy link

sosheskaz commented Oct 30, 2023

I would also like to see this get merged.

I do not know much about terraform programming or debugging, but I can see:

It seems to me like it is using the OU ID(s) to determine whether or not the deployment target has changed, in which case, that "ID" would probably need to change to include the filter operation and the account ID, for CRUD and idempotence purposes.

If my theory is correct, then I would expect the addition of resources to result in a "Nothing to do" change, because the deployment target "remained the same" because the presence of accounts are not factored into change detection. However, if something else were to trigger a change (a change in the Stackset?), from reading the code, I would expect the update operation to complete successfully and alter the deployment targets. Can anyone support whether my speculation might be valid?

@JaimePoloTeck
Copy link

Really need this to get going, this is a basic feature and has been going on for about a year, any ETA for when this will be merged?

@yhamano0312
Copy link
Contributor

I have submitted a PR to ensure that the resource is updated even when changes are made to the deployment_targets argument.
#37898

@ewbankkit
Copy link
Contributor

This functionality was included in #37898.

@ewbankkit ewbankkit closed this Jul 23, 2024
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/cloudformation Issues and PRs that pertain to the cloudformation service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Add additional configuration parameters to resource:aws_cloudformation_stack_set_instance
7 participants