-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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, | ||
}, | ||
|
||
"notification_type": &schema.Schema{ | ||
Type: schema.TypeSet, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be of type |
||
Required: true, | ||
}, | ||
|
||
"identity": &schema.Schema{ | ||
Type: schema.TypeSet, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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), | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
_, err := conn.SetIdentityNotificationTopicRequest(setOpts).Send() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should always call the non-request method (e.g. |
||
|
||
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)}, | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
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": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
if err := d.Set("topic", notificationAttributes.BounceTopic); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that this Also, it could be really interesting to add a |
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To delete it, I think we should call the |
||
return resourceAwsSesNotificationSet(d, meta) | ||
} |
There was a problem hiding this comment.
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 thanRequired: true
: even though the logic is somehow the same, it's easier to read the code 😄There was a problem hiding this comment.
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!