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

resource/aws_sns_topic: Fix exit after updating first attribute #3360

Merged
merged 2 commits into from
Feb 16, 2018
Merged

resource/aws_sns_topic: Fix exit after updating first attribute #3360

merged 2 commits into from
Feb 16, 2018

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Feb 13, 2018

Closes #3354

😬 This bug was very subtle as nothing was calling d.Set(attribute, "") on attributes not existing in AWS. This bug has been in the codebase since v1.0.0 of the provider. Also did some other random cleanup here.

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopic_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSNSTopic_ -timeout 120m
=== RUN   TestAccAWSSNSTopic_importBasic
--- PASS: TestAccAWSSNSTopic_importBasic (11.84s)
=== RUN   TestAccAWSSNSTopic_basic
--- PASS: TestAccAWSSNSTopic_basic (9.46s)
=== RUN   TestAccAWSSNSTopic_name
--- PASS: TestAccAWSSNSTopic_name (10.60s)
=== RUN   TestAccAWSSNSTopic_namePrefix
--- PASS: TestAccAWSSNSTopic_namePrefix (9.19s)
=== RUN   TestAccAWSSNSTopic_policy
--- PASS: TestAccAWSSNSTopic_policy (10.33s)
=== RUN   TestAccAWSSNSTopic_withIAMRole
--- PASS: TestAccAWSSNSTopic_withIAMRole (30.35s)
=== RUN   TestAccAWSSNSTopic_withFakeIAMRole
--- PASS: TestAccAWSSNSTopic_withFakeIAMRole (66.95s)
=== RUN   TestAccAWSSNSTopic_withDeliveryPolicy
--- PASS: TestAccAWSSNSTopic_withDeliveryPolicy (10.52s)
=== RUN   TestAccAWSSNSTopic_deliveryStatus
--- PASS: TestAccAWSSNSTopic_deliveryStatus (29.91s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	189.199s

@bflad bflad added bug Addresses a defect in current functionality. service/sns Issues and PRs that pertain to the sns service. labels Feb 13, 2018
@bflad bflad added this to the v1.10.0 milestone Feb 13, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 13, 2018
@bflad bflad mentioned this pull request Feb 13, 2018
Closed
radeksimko
radeksimko approved these changes Feb 16, 2018
View reviewed changes
aws/resource_aws_sns_topic.go
_, terraformAttrValue := d.GetChange(terraformAttrName)
err := updateAwsSnsTopicAttribute(d.Id(), snsAttrName, terraformAttrValue, conn)
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

Thank you, this looks so much cleaner! 👍 🚿

aws/resource_aws_sns_topic.go
"sqs_failure_feedback_role_arn": "SQSFailureFeedbackRoleArn",
"sqs_success_feedback_role_arn": "SQSSuccessFeedbackRoleArn",
"sqs_success_feedback_sample_rate": "SQSSuccessFeedbackSampleRate",
}
Copy link
Member

Choose a reason for hiding this comment

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

I kind of wish this map wasn't here at all, but there's only so much refactoring that can be done in a single PR... 😄

radeksimko
radeksimko reviewed Feb 16, 2018
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Actually, do you mind attaching a test with the config from #3354 ? Then it's 👌

@bflad
Copy link
Contributor Author

bflad commented Feb 16, 2018

No problem, certainly a valid ask!

@bflad
test/resource/aws_sns_topic: Add testAccCheckAWSSNSTopicAttributes he…
ce07621 
…lper and verify TestAccAWSSNSTopic_deliveryStatus fix
@bflad
Copy link
Contributor Author

bflad commented Feb 16, 2018

@radeksimko I've now made it so testAccCheckAWSSNSTopicExists extracts the attributes it finds from AWS and introduced a testAccCheckAWSSNSTopicAttributes function to verify them against a set map. Now this should be convincing of the fix! 😂

Previously (note 11 errors with 12 attributes submitted, I also need to submit a PR to go-multierror to make it sortable 😉 ):

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopic_deliveryStatus'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSNSTopic_deliveryStatus -timeout 120m
=== RUN   TestAccAWSSNSTopic_deliveryStatus
--- FAIL: TestAccAWSSNSTopic_deliveryStatus (8.72s)
	testing.go:513: Step 0 error: Check failed: Check 2/14 error: 11 error(s) occurred:

		* expected SNS topic attribute "ApplicationSuccessFeedbackRoleArn" to match "^arn:aws:iam::[0-9]{12}:role/sns-delivery-status-role-", received: ""
		* expected SNS topic attribute "HTTPSuccessFeedbackSampleRate" to match "^80$", received: ""
		* expected SNS topic attribute "LambdaSuccessFeedbackSampleRate" to match "^90$", received: ""
		* expected SNS topic attribute "LambdaSuccessFeedbackRoleArn" to match "^arn:aws:iam::[0-9]{12}:role/sns-delivery-status-role-", received: ""
		* expected SNS topic attribute "SQSFailureFeedbackRoleArn" to match "^arn:aws:iam::[0-9]{12}:role/sns-delivery-status-role-", received: ""
		* expected SNS topic attribute "ApplicationSuccessFeedbackSampleRate" to match "^100$", received: ""
		* expected SNS topic attribute "ApplicationFailureFeedbackRoleArn" to match "^arn:aws:iam::[0-9]{12}:role/sns-delivery-status-role-", received: ""
		* expected SNS topic attribute "HTTPSuccessFeedbackRoleArn" to match "^arn:aws:iam::[0-9]{12}:role/sns-delivery-status-role-", received: ""
		* expected SNS topic attribute "HTTPFailureFeedbackRoleArn" to match "^arn:aws:iam::[0-9]{12}:role/sns-delivery-status-role-", received: ""
		* expected SNS topic attribute "LambdaFailureFeedbackRoleArn" to match "^arn:aws:iam::[0-9]{12}:role/sns-delivery-status-role-", received: ""
		* expected SNS topic attribute "SQSSuccessFeedbackRoleArn" to match "^arn:aws:iam::[0-9]{12}:role/sns-delivery-status-role-", received: ""
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	8.766s

Now:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopic_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSNSTopic_ -timeout 120m
=== RUN   TestAccAWSSNSTopic_importBasic
--- PASS: TestAccAWSSNSTopic_importBasic (10.36s)
=== RUN   TestAccAWSSNSTopic_basic
--- PASS: TestAccAWSSNSTopic_basic (14.89s)
=== RUN   TestAccAWSSNSTopic_name
--- PASS: TestAccAWSSNSTopic_name (10.43s)
=== RUN   TestAccAWSSNSTopic_namePrefix
--- PASS: TestAccAWSSNSTopic_namePrefix (9.13s)
=== RUN   TestAccAWSSNSTopic_policy
--- PASS: TestAccAWSSNSTopic_policy (11.53s)
=== RUN   TestAccAWSSNSTopic_withIAMRole
--- PASS: TestAccAWSSNSTopic_withIAMRole (20.08s)
=== RUN   TestAccAWSSNSTopic_withFakeIAMRole
--- PASS: TestAccAWSSNSTopic_withFakeIAMRole (67.97s)
=== RUN   TestAccAWSSNSTopic_withDeliveryPolicy
--- PASS: TestAccAWSSNSTopic_withDeliveryPolicy (11.40s)
=== RUN   TestAccAWSSNSTopic_deliveryStatus
--- PASS: TestAccAWSSNSTopic_deliveryStatus (29.90s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	185.739s

@bflad bflad merged commit 71e9b48 into master Feb 16, 2018
@bflad bflad deleted the b-aws_sns_topic-less-magic branch February 16, 2018 23:51
bflad added a commit that referenced this pull request Feb 16, 2018
@bflad
Update CHANGELOG for #3360
e3b21df
@bflad
Copy link
Contributor Author

bflad commented Feb 27, 2018

This has been released in version 1.10.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 7, 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 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radeksimko radeksimko radeksimko approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. service/sns Issues and PRs that pertain to the sns service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.10.0
Development

Successfully merging this pull request may close these issues.

AWS SNS delivery failure feedback not set
2 participants
@bflad @radeksimko