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

Issue #5808 aws_iam_policy does not have a description attribute #5815

Merged
merged 3 commits into from
Sep 13, 2018

Conversation

saravanan30erd
Copy link
Contributor

Fixes #5808

Changes proposed in this pull request:

  • Change 1
    added default value to description attribute.

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAWSPolicy_checkDescription'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAWSPolicy_checkDescription -timeout 120m
=== RUN   TestAWSPolicy_checkDescription
--- PASS: TestAWSPolicy_checkDescription (28.46s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	28.503s
...

@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Sep 8, 2018
@bflad bflad added breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. service/iam Issues and PRs that pertain to the iam service. labels Sep 11, 2018
@bflad
Copy link
Contributor

bflad commented Sep 11, 2018

Hi @saravanan30erd 👋 Thanks for submitting this.

Changing the attribute default in this manner is a breaking change for existing Terraform configurations (even with a state migration) and while it accomplishes the goal of having d.Set("description", ...) always called (since it will not be returned as nil by default from the API), its not necessarily the correct way to accomplish this.

This is the problematic line, which dates back 3 years:

https://github.com/terraform-providers/terraform-provider-aws/blob/7be15b7d6273e2badbd0a7d2453eee452bcecd7c/aws/resource_aws_iam_policy.go#L281

My best guess is that this was written before d.Set() was setup to handle pointers for scalar types (e.g. *string) and that the other fields were fine without that check because they are more or less "required" from a service standpoint. Nowadays, we can pass a *string directly into d.Set() and it'll set "" in the state if its an empty string.

Instead of making the broader changes here, I would suggest simply trying to just remove that if policy.Description != nil line and adding a check to an existing acceptance test like this (where description isn't being set in test configuration):

resource.TestCheckResourceAttr("aws_iam_policy.policy", "description", ""),

Theoretically, the above check should fail on master for test configurations missing that argument.

@bflad bflad added waiting-response Maintainers are waiting on response from community or contributor. bug Addresses a defect in current functionality. labels Sep 11, 2018
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Sep 12, 2018
@saravanan30erd
Copy link
Contributor Author

saravanan30erd commented Sep 12, 2018

@bflad thanks, done 👍
I referred how description handled in some other resources and followed the same.
But it will affect the existing configurations as you said, I ll note it.

@bflad bflad removed breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. waiting-response Maintainers are waiting on response from community or contributor. labels Sep 13, 2018
@bflad bflad added this to the v1.36.0 milestone Sep 13, 2018
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.

🎉 Glad this worked out and thanks for fixing this! 🚀

2 tests passed (all tests)
--- PASS: TestAWSPolicy_invalidJson (0.56s)
--- PASS: TestAWSPolicy_namePrefix (6.72s)

@bflad bflad merged commit e8d1a94 into hashicorp:master Sep 13, 2018
bflad added a commit that referenced this pull request Sep 13, 2018
@bflad
Copy link
Contributor

bflad commented Sep 13, 2018

This has been released in version 1.36.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 3, 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 and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/iam Issues and PRs that pertain to the iam service. size/XS Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_iam_policy does not have a description attribute
2 participants