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

[WIP/Request Review] New Resource: aws_ses_identity_notification_topic #946

Closed
wants to merge 3 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 95 additions & 0 deletions aws/resource_aws_ses_identity_notification.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package aws

import (
"fmt"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ses"
"github.com/hashicorp/terraform/helper/schema"
)

func resourceAwsSesNotification() *schema.Resource {
return &schema.Resource{
Create: resourceAwsSesNotificationSet,
Read: resourceAwsSesNotificationRead,
Update: resourceAwsSesNotificationSet,
Delete: resourceAwsSesNotificationDelete,

Schema: map[string]*schema.Schema{
"topic_arn": &schema.Schema{
Type: schema.TypeString,
Required: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to set Optional: true rather than Required: true: even though the logic is somehow the same, it's easier to read the code 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we need to add a ValidateFunc: validateArn, attribute, so that it uses a function to validate the value passed in the configuration. validateArn is an existing function that validates the format using a given RegExp, so you don't nee much more here :)

For other attributes, it can be nice to also validate the value passed, using custom functions!

},

"notification_type": &schema.Schema{
Type: schema.TypeSet,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be of type TypeString rather than TypeSet. A set is a composition of fields, which can be uniquely identified based on its hash. Moreover, you would have issues when compiling as you try to get the string pointer value line 45

Required: true,
},

"identity": &schema.Schema{
Type: schema.TypeSet,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be of type TypeString rather than TypeSet. A set is a composition of fields, which can be uniquely identified based on its hash. Moreover, you would have issues when compiling as you try to get the string pointer value line 44

Required: true,
},
},
}
}

func resourceAwsSesNotificationSet(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).sesConn
topic := d.Get("topic_arn").(string)
notification := d.Get("notification_type").(string)
identity := d.Get("identity").(string)

setOpts := &ses.SetIdentityNotificationTopicInput{
Identity: aws.String(identity),
NotificationType: aws.String(notification),
SnsTopic: aws.String(topic),
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be really nice to add some debug here, using something in the idea:

log.Printf("[DEBUG] Setting SES Identity Notification: %#v", setOpts)

_, err := conn.SetIdentityNotificationTopicRequest(setOpts).Send()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should always call the non-request method (e.g. SetIdentityNotificationTopic), because it handles some logic for us


if err != nil {
return fmt.Errorf("Error setting SES Identity Notification: %s", err)
}

return resourceAwsSesNotificationRead(d, meta)
}

func resourceAwsSesNotificationRead(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).sesConn
notification := d.Get("notification_type").(*schema.Set)
identity := d.Get("identity").(*schema.Set)

getOpts := &ses.GetIdentityNotificationAttributesInput{
Identities: []*string{aws.String(identity)},
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be really nice to add some debug here, using something in the idea:

log.Printf("[DEBUG] Reading SES Identity Notification Attributes: %#v", getOpts)

response, err := conn.GetIdentityNotificationAttributes(getOpts)

if err != nil {
return fmt.Errorf("Error reading SES Identity Notification: %s", err)
}

notificationAttributes := response.NotificationAttributes[identity]
switch notification {
case "Bounce":
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be nice to rely on the constants provided by the SDK in places where we can used them, so that we avoid any typo.
For instance, using ses.NotificationTypeBounce, ses.NotificationTypeComplain, etc.

if err := d.Set("topic", notificationAttributes.BounceTopic); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that this topic field does not exist in the schema. Did you think of something or is it a typo?

Also, it could be really interesting to add a attributes field, which would contain everything retrieved/sent back by the AWS API when reading the field. Thoughts? :)

return err
}
case "Complain":
if err := d.Set("topic", notificationAttributes.ComplaintTopic); err != nil {
return err
}
case "Delivery":
if err := d.Set("topic", notificationAttributes.DeliveryTopic); err != nil {
return err
}
}

return nil
}

func resourceAwsSesNotificationDelete(d *schema.ResourceData, meta interface{}) error {
d.Set("topic_arn", nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

To delete it, I think we should call the SetIdentityNotificationTopic method, passing nil / empty for the SNS Topic. Thoughts? :)

return resourceAwsSesNotificationSet(d, meta)
}