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.

Sign up for GitHub

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

Jump to bottom

Prevent importing Organization policy #15446

Merged
merged 7 commits into from
Oct 6, 2020
Merged

Prevent importing Organization policy #15446

merged 7 commits into from
Oct 6, 2020

Conversation

gdavison
Copy link
Contributor

@gdavison gdavison commented Oct 2, 2020

Prevent importing Organization policy

Closes #15397

Release note for CHANGELOG:

* resource/aws_organizations_policy: Prevent errors with imported AWS-managed Organizations policies

Output from acceptance testing:

$ make testacc TESTARGS="-run=TestAccAWSOrganizations_serial/Policy/"

--- PASS: TestAccAWSOrganizations_serial (850.06s)
    --- PASS: TestAccAWSOrganizations_serial/Policy (654.98s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Tags (151.40s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Type_Backup (55.62s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/concurrent (58.21s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Description (88.82s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Type_AI_OPT_OUT (54.79s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Type_SCP (80.41s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Type_Tag (48.54s)
        --- SKIP: TestAccAWSOrganizations_serial/Policy/ImportAwsManagedPolicy (0.00s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/basic (77.75s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/disappears (39.43s)
    --- PASS: TestAccAWSOrganizations_serial/PolicyAttachment (195.08s)
        --- PASS: TestAccAWSOrganizations_serial/PolicyAttachment/Account (85.64s)
        --- PASS: TestAccAWSOrganizations_serial/PolicyAttachment/OrganizationalUnit (57.27s)
        --- PASS: TestAccAWSOrganizations_serial/PolicyAttachment/Root (52.18s)

@gdavison gdavison requested a review from a team October 2, 2020 07:20
@ghost ghost added size/L Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. service/organizations Issues and PRs that pertain to the organizations service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 2, 2020
bflad added a commit that referenced this pull request Oct 2, 2020
@bflad
tests/provider: Support (helper/schema.Resource).DeleteContext in tes…
8c33ed6 
…tAccCheckResourceDisappears

Reference: #15446
@bflad bflad mentioned this pull request Oct 2, 2020
Merged
bflad
bflad suggested changes Oct 2, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Please reach out if you have any questions.

aws/resource_aws_organizations_policy.go Outdated Show resolved Hide resolved
aws/resource_aws_organizations_policy.go Outdated
}

func resourceAwsOrganizationsPolicyRead(d *schema.ResourceData, meta interface{}) error {
func resourceAwsOrganizationsPolicyRead(ctx context.Context, d *schema.ResourceData, meta interface{}) (diags diag.Diagnostics) {
Copy link
Contributor

@bflad bflad Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Since this is the first introduction of context-aware resource functions, we will want to set a good standard. 😄

Recommendations in Go's tutorial and Go's Code Review Comments suggest to only use named result parameters in shorter number of line functions or if there is ambiguity between multiple return values of the same type. To prevent this going forward we can fix the reports and enable the nakedret linter in golangci-lint.

For functions that want to build multiple diagnostics before returning, they can instantiate a var diags diag.Diagnostics. 👍

Suggested change
func resourceAwsOrganizationsPolicyRead(ctx context.Context, d *schema.ResourceData, meta interface{}) (diags diag.Diagnostics) {
func resourceAwsOrganizationsPolicyRead(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {

aws/resource_aws_organizations_policy.go Outdated Show resolved Hide resolved
aws/resource_aws_organizations_policy.go Show resolved Hide resolved
tests/resource_aws_organizations_policy/.terraform-version Outdated
@@ -0,0 +1 @@
0.13.4
Copy link
Contributor

@bflad bflad Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This new testing is nice because its covering this very specific scenario, but I have a few concerns:

  • This testing is outside of currently automated test runs (when would this get run again? when should it?)
  • This new directory structure and testing style is not self-documented, in the contributing/maintaining guide, or in the Extending Terraform documentation (when should this be used? why is this here?)
  • Introduces a new maintenance burden of maintaining a Terraform version here (is the version number significant? should it instead be in the configuration as required_version = ">= 0.12"?)

I think we should probably work through some of these before introducing it into the codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure. I included it here essentially to "show my work" with testing the change. I expected it to be iterated on before becoming something official. I'll pull it out and make a WIP proposal

tests/resource_aws_organizations_policy/run_test.sh Outdated Show resolved Hide resolved
bflad added a commit that referenced this pull request Oct 2, 2020
@bflad
tests/provider: Support (helper/schema.Resource).DeleteContext in tes…
9cd3645 
…tAccCheckResourceDisappears (#15448)

Reference: #15446
bflad
bflad reviewed Oct 2, 2020
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
bflad
bflad approved these changes Oct 2, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Otherwise, looks good 🚀

Output from acceptance testing:

    --- PASS: TestAccAWSOrganizations_serial/Policy (313.22s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/concurrent (32.45s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Type_AI_OPT_OUT (22.80s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Type_Backup (22.51s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Type_Tag (23.26s)
        --- SKIP: TestAccAWSOrganizations_serial/Policy/ImportAwsManagedPolicy (0.00s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/basic (38.80s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Description (39.69s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Tags (76.30s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/disappears (19.64s)
        --- PASS: TestAccAWSOrganizations_serial/Policy/Type_SCP (37.78s)
    --- PASS: TestAccAWSOrganizations_serial/PolicyAttachment (103.45s)
        --- PASS: TestAccAWSOrganizations_serial/PolicyAttachment/Account (47.89s)
        --- PASS: TestAccAWSOrganizations_serial/PolicyAttachment/OrganizationalUnit (28.38s)
        --- PASS: TestAccAWSOrganizations_serial/PolicyAttachment/Root (27.18s)

@bflad bflad added the bug Addresses a defect in current functionality. label Oct 2, 2020
@gdavison @bflad
Update .gitignore
460a429 
Co-authored-by: Brian Flad <[email protected]>
@gdavison gdavison added this to the v3.10.0 milestone Oct 6, 2020
@gdavison gdavison merged commit 813ed58 into master Oct 6, 2020
@gdavison gdavison deleted the b-prevent-importing-organization-policy branch October 6, 2020 21:04
gdavison added a commit that referenced this pull request Oct 6, 2020
@gdavison
Update CHANGELOG for #15446
d0b731b
@gdavison gdavison mentioned this pull request Oct 7, 2020
Merged
@ghost
Copy link

ghost commented Oct 9, 2020

This has been released in version 3.10.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!

@ghost
Copy link

ghost commented Nov 6, 2020

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!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. provider Pertains to the provider itself, rather than any interaction with AWS. service/organizations Issues and PRs that pertain to the organizations service. size/L 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
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.

Prevent importing AWS-managed Organizations policy
2 participants
@gdavison @bflad