-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
adding fix for aws org feature_set update issue from #15462 #15473
adding fix for aws org feature_set update issue from #15462 #15473
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryandeivert I'm not a maintainer so feel free to ignore, but looks like feature_set changes aren't yet implemented in the Read/Update functions, so this doesn't change the infra in AWS. Might need something like (I'm quite new to TF and Go so apologies if the below examples are garbage):
// somewhere within the Read function
if err := d.Set("feature_set", org.Organization.FeatureSet); err != nil {
return fmt.Errorf("error setting feature_set: %s", err)
}
// somewhere within the Update function
if d.HasChange("feature_set") {
o, n := d.GetChange("feature_set")
input := &organizations.EnableAllFeaturesInput{}
if _, err := conn.EnableAllFeatures(input); err != nil {
return fmt.Errorf("error changing feature_set from (%s) to (%s): %s", o, n, err)
}
}
Also may be worth updating docs, since when you enable "All" you still have to manually go into the AWS console and click "Finalize" once all accounts have accepted the change. Until you Finalize it's left in a state where you can still revert things.
good catch @philnichol - I added some updates and fixed a different bug in the current tests. docs are also updated now |
@ryandeivert great stuff! |
hi @anGie44 I noticed you updated the issue associated with this. is there any chance you can provide an update here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ryandeivert , thank you for this PR and your patience! Overall changes look great just some small changes needed.
hi @anGie44 - thank you for the review! I believe I have addressed all of your feedback now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM 🚀 Thanks again @ryandeivert @philnichol !
Output of acceptance tests:
--- PASS: TestFlattenOrganizationsRoots (0.00s)
--- PASS: TestAccAWSOrganizations_serial (1103.64s)
--- PASS: TestAccAWSOrganizations_serial/Organization (217.24s)
--- PASS: TestAccAWSOrganizations_serial/Organization/FeatureSet_ForcesNew (26.15s)
--- PASS: TestAccAWSOrganizations_serial/Organization/DataSource (25.22s)
--- PASS: TestAccAWSOrganizations_serial/Organization/basic (12.52s)
--- PASS: TestAccAWSOrganizations_serial/Organization/AwsServiceAccessPrincipals (30.70s)
--- PASS: TestAccAWSOrganizations_serial/Organization/EnabledPolicyTypes (90.92s)
--- PASS: TestAccAWSOrganizations_serial/Organization/FeatureSet_Basic (11.43s)
--- PASS: TestAccAWSOrganizations_serial/Organization/FeatureSet_Update (20.30s)
--- PASS: TestAccAWSOrganizations_serial/Account (0.00s)
--- SKIP: TestAccAWSOrganizations_serial/Account/basic (0.00s)
--- SKIP: TestAccAWSOrganizations_serial/Account/ParentId (0.00s)
--- SKIP: TestAccAWSOrganizations_serial/Account/Tags (0.00s)
--- PASS: TestAccAWSOrganizations_serial/OrganizationalUnit (38.24s)
--- PASS: TestAccAWSOrganizations_serial/OrganizationalUnit/basic (14.78s)
--- PASS: TestAccAWSOrganizations_serial/OrganizationalUnit/Name (23.45s)
--- PASS: TestAccAWSOrganizations_serial/OrganizationalUnits (0.00s)
--- PASS: TestAccAWSOrganizations_serial/OrganizationalUnits/DataSource (13.49s)
--- PASS: TestAccAWSOrganizations_serial/Policy (764.26s)
--- PASS: TestAccAWSOrganizations_serial/Policy/Description (24.70s)
--- PASS: TestAccAWSOrganizations_serial/Policy/Tags (321.10s)
--- PASS: TestAccAWSOrganizations_serial/Policy/Type_SCP (296.24s)
--- PASS: TestAccAWSOrganizations_serial/Policy/Type_Tag (13.83s)
--- PASS: TestAccAWSOrganizations_serial/Policy/basic (25.28s)
--- PASS: TestAccAWSOrganizations_serial/Policy/concurrent (25.83s)
--- PASS: TestAccAWSOrganizations_serial/Policy/disappears (13.14s)
--- PASS: TestAccAWSOrganizations_serial/Policy/Type_AI_OPT_OUT (15.99s)
--- PASS: TestAccAWSOrganizations_serial/Policy/Type_Backup (13.66s)
--- PASS: TestAccAWSOrganizations_serial/Policy/ImportAwsManagedPolicy (14.49s)
--- PASS: TestAccAWSOrganizations_serial/PolicyAttachment (70.41s)
--- PASS: TestAccAWSOrganizations_serial/PolicyAttachment/OrganizationalUnit (19.30s)
--- PASS: TestAccAWSOrganizations_serial/PolicyAttachment/Root (18.89s)
--- PASS: TestAccAWSOrganizations_serial/PolicyAttachment/Account (32.22s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 1106.033s
This has been released in version 3.16.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Community Note
Closes #15462
Release note for CHANGELOG:
Output from acceptance testing: