resource/aws_ses_event_destination: Add support for SNS destinations #1737
Conversation
Fixes: hashicorp#1697 ``` terraform-provider-aws [master●] % acctests aws TestAccAWSSESEventDestination_basic === RUN TestAccAWSSESEventDestination_basic --- PASS: TestAccAWSSESEventDestination_basic (159.86s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 159.898s ```
stack72
force-pushed
the
b-aws-ses-destination-1697
branch
from
54cdfa0
to
ecdc749
Compare
| @@ -156,6 +158,10 @@ data "aws_iam_policy_document" "fh_felivery_document" { | |||
| } | |||
| } | |||
|
|
|||
| resource "aws_sns_topic" "ses_destination" { | |||
| name = "ses-destination-test" | |||
There was a problem hiding this comment.
Do you mind randomizing this name here?
| default_value = "default" | ||
| dimension_name = "dimension" | ||
| value_source = "emailHeader" | ||
| } | ||
| } | ||
|
|
||
| resource "aws_ses_event_destination" "sns" { | ||
| name = "event-destination-sns", |
There was a problem hiding this comment.
Likewise ^ do you mind randomizing it?
| @@ -26,6 +26,8 @@ func TestAccAWSSESEventDestination_basic(t *testing.T) { | |||
| "aws_ses_event_destination.kinesis", "name", "event-destination-kinesis"), | |||
| resource.TestCheckResourceAttr( | |||
| "aws_ses_event_destination.cloudwatch", "name", "event-destination-cloudwatch"), | |||
| resource.TestCheckResourceAttr( | |||
| "aws_ses_event_destination.sns", "name", "event-destination-sns"), | |||
There was a problem hiding this comment.
Nitpick - but do you mind adding check for sns_destination.0.topic_arn as that is the field being added and that's the one that should also be tested?
radeksimko
added
bug
|
@radeksimko so I looked at this and the reason the original tests are not actually randomized is that there is nothing happening in the Read func - therefore, when we check that a destroy happens, we are actually checking the hardcoded SES destination to make sure it no longer exists Thoughts? P. |
|
@radeksimko Is this dead in the water? I would love to see this in the next release... |
|
@radeksimko +1 need this! |
radeksimko
added
the
size/M
|
@stack72 I have what might be valid tests for what @radeksimko has asked for - how best to get them into this PR? |
Add random digits to SNS event and topic resources Add check for correct SNS destination ARN Rename poorly named test adjust log msgs for same
There was a problem hiding this comment.
Thanks for all the work here @stack72
I just added the randomization & pushed to your branch. I hope you don't mind.
=== RUN TestAccAWSSESEventDestination_basic
--- PASS: TestAccAWSSESEventDestination_basic (206.21s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 206.240s
|
There are other things to improve on that resource - e.g. turning all the Merging now... |
|
This has been released in terraform-provider-aws version 1.7.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
| @@ -53,20 +53,21 @@ func resourceAwsSesEventDestination() *schema.Resource { | |||
| Type: schema.TypeSet, | |||
| Optional: true, | |||
| ForceNew: true, | |||
| ConflictsWith: []string{"kinesis_destination"}, | |||
| MaxItems: 1, | |||
There was a problem hiding this comment.
Interestingly enough, this resource actually did appear to support multiple cloudwatch_destination previously, even if it wasn't appropriately acceptance tested. This change will likely be reverted in #6690
|
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
locked and limited conversation to collaborators
Fixes: #1697