New Resource: aws_ses_identity_notification #2640
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey @hussfelt, Thanks for continuing this! Unfortunately havent had much time at work to continue the work I started. I was actually planning to pick it up this week but looks like you have done that for me 🤣 If there is any more feedback, I should be able to help on this in a couple of days. Cheers, Ben |
|
@benoj Thanks! I'll try to complete it - if I need backup I'll ping you here and open up contributions to my branch :-) Thank you for starting it - I would not have done it with my limited golang knowledge :) |
|
@Ninir or anyone checking this - I am in need of being pointed in the right direction. My tests seem to be passing - not sure they make sense though. The |
|
Seems like tests are passing, fairly sure we can improve this code though. Awaiting feedback :) |
|
Any word on this PR? |
Ninir
added
the
enhancement
|
Ping, any feedback is appreciated - we'd love to get this enhancement in :) |
|
echo :) |
|
+1 for this :) |
|
+1 👍 |
|
@hussfelt maybe you should remove the WIP from the subject |
hussfelt
changed the title
|
@benoj DONE! :) |
|
ping @Ninir :) |
radeksimko
added
new-resource
radeksimko
changed the title
|
Guys this has been in PR (original PR) since June 22 - can we please get this in. |
bflad
reviewed
Mar 29, 2018
There was a problem hiding this comment.
Hi there -- sorry for the long maintainer silence on this pull request and thank you very much for the contribution. 😅 I left some initial feedback for this new resource below. Besides the comments below, there is other missing pieces here:
- Documentation page:
website/docs/r/ses_identity_notification.html.markdown - A new link to the documentation page in
website/aws.erb - Adding the resource to the provider in
aws/provider.go(mappingaws_ses_identity_notificationtoresourceAwsSesNotification)
Please take a look and let me know if you have any questions or do not have time to implement the feedback. Thanks.
| Delete: resourceAwsSesNotificationDelete, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "topic_arn": &schema.Schema{ |
There was a problem hiding this comment.
Minor nitpick: &schema.Schema here and on the attributes below is extraneous as its already declared above in map[string]*schema.Schema
| "notification_type": &schema.Schema{ | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| ValidateFunc: validateNotificationType, |
There was a problem hiding this comment.
Minor nitpick: the extra function can be removed by using:
ValidateFunc: validation.StringInSlice([]string{
ses.NotificationTypeBounce,
ses.NotificationTypeComplaint,
ses.NotificationTypeDelivery,
}, false),| ValidateFunc: validateArn, | ||
| }, | ||
|
|
||
| "notification_type": &schema.Schema{ |
There was a problem hiding this comment.
We should set ForceNew: true on this attribute as updating it could create situations where the old notification still exists
| ValidateFunc: validateNotificationType, | ||
| }, | ||
|
|
||
| "identity": &schema.Schema{ |
There was a problem hiding this comment.
We should set ForceNew: true on this attribute as updating it could create situations where the old notification still exists
| "identity": &schema.Schema{ | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| ValidateFunc: validateIdentity, |
There was a problem hiding this comment.
Minor nitpick: the extra function can be removed by using:
ValidateFunc: validation.NoZeroValues,| } | ||
| } | ||
|
|
||
| func resourceAwsSesNotificationSet(d *schema.ResourceData, meta interface{}) error { |
There was a problem hiding this comment.
We need to call d.SetId() in here so we can reference the resource identifier in the read function and testing. I would recommend creating a two part ID such as d.SetId(fmt.Sprintf("%s|%s", identity, notification))
This will also allow us to later implement importing.
|
|
||
| func resourceAwsSesNotificationRead(d *schema.ResourceData, meta interface{}) error { | ||
| conn := meta.(*AWSClient).sesConn | ||
| notification := d.Get("notification_type").(string) |
There was a problem hiding this comment.
Ideally, we should be reading this information from the Terraform resource ID via d.Id(). If implemented as noted above, this can be accomplished via:
identity, notificationType, err := decodeSesIdentityNotificationId(d.Id())
if err != nil {
return err
}And the function is implemented like:
func decodeSesIdentityNotificationId(id string) (string, string, error) {
parts := strings.Split(id, "|")
if len(parts) != 2 {
return "", "", fmt.Errorf("Unexpected format of ID (%q), expected IDENTITY|TYPE", id)
}
return parts[0], parts[1], nil
}| notificationAttributes := response.NotificationAttributes[identity] | ||
| switch notification { | ||
| case ses.NotificationTypeBounce: | ||
| if err := d.Set("topic_arn", notificationAttributes.BounceTopic); err != nil { |
There was a problem hiding this comment.
For d.Set() with simple *string input, we can skip the err checking here and below. 👍
|
|
||
| func resourceAwsSesNotificationDelete(d *schema.ResourceData, meta interface{}) error { | ||
| conn := meta.(*AWSClient).sesConn | ||
| notification := d.Get("notification_type").(string) |
There was a problem hiding this comment.
Same note here about using d.Id()
| }, | ||
| Providers: testAccProviders, | ||
| CheckDestroy: testAccCheckAwsSESIdentityNotificationDestroy, | ||
| Steps: []resource.TestStep{ |
There was a problem hiding this comment.
We should have a TestStep that attempts to update topic_arn
There was a problem hiding this comment.
@bflad I fixed everything but this. I do not know how :-)
There was a problem hiding this comment.
@bflad could you point me to an example on how to accomplish this - elsewhere in the codebase?
There was a problem hiding this comment.
@bflad added a case where I push an updated arn. Hope this is the correct implementation.
bflad
added
the
waiting-response
|
@bflad I noticed some errors - i'll fix those in the morning :) |
|
I seem to have pushed commits to this PR that wasn't mine, by rebasing over my local master. I have seen this before - unclear for me how to correct this... Anyone? If I issue a new PR it seems to be clean. But then the I am afraid it will end up in another 6 months of silence... :-) |
|
@bflad fixed! Seems to take some time though? Specifying |
|
@bflad before I run this again for over 4000 seconds, please let me know that this is expected. |
|
@bflad yeah, so I have run this 3 times now and each time takes over an hour. :) It's not practical to finish the acceptance test this way... :) This is my current error. I added an empty Please help me speed up the test or help me understand why this fails! :) |
|
And just as I gave up I realised that this is probably the cause: Adding a check on the string and passing "nil" if empty. |
|
You can turn on debug logging with TF_LOG=debug to see why its just hanging. It might be permissions issue or an error that keeps retrying. The easiest way to handle the topic might be removing it from always being defined in v, ok := d.GetOk("topic_arn"); ok && v.(string) != "" {
setOpts.SnsTopic = aws.String(v.(string))
} |
|
@bflad thanks. Yes, repeating requests. 500 error. I'm hitting a wall now though. |
|
It might be worth contacting AWS Support about those 500's. Do they also happen in |
|
@bflad unfortunately yes. Same error in |
|
@bflad I am starting to believe that I got this wrong. Could it be that the resources are named wrong and thus connecting to the wrong API endpoints? This is the response from AWS: |
|
@bflad the rename has nothing to do with it, but makes sense. |
|
@bflad found the cause, I am passing our concatenated ID into the API, which also contains the type. |
|
@bflad Acceptance test passing: Hope we're done :) |
bflad
removed
the
waiting-response
bflad
approved these changes
Apr 4, 2018
|
🎉! Thanks! |
bflad
added a commit
that referenced
this pull request
Apr 4, 2018
|
@hussfelt thanks for finishing this! Definitely took some massaging :) |
|
This has been released in version 1.14.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This refs #931 and stalled PR #946
As I am unable to push to that branch I based my changes on @benoj's work and applied the changes based on feedback from @Ninir.
The tests has not yet run, and the test-file is pseudo code only.Working on getting a go-environment running.
This is my first time touching golang for real - so I am pretty much in the dark here :-)
Reference: https://docs.aws.amazon.com/ses/latest/APIReference/API_SetIdentityNotificationTopic.html?shortFooter=true