[WIP/Request Review] New Resource: aws_ses_identity_notification_topic #946
Conversation
There was a problem hiding this comment.
Hi @benoj
Sorry for the long silence, we failed terribly here!
Just made a first review of the work! there are a few things to address, but this is getting into shape :)
Also, it would be nice to add acceptance testing (you can have a look at other *_test.go files in the aws package, as in resource_aws_cloudwatch_dashboard_test.go) and the related documentation! 🚀
Do you need more help on testing?
Keep going!
| SnsTopic: aws.String(topic), | ||
| } | ||
|
|
||
| _, err := conn.SetIdentityNotificationTopicRequest(setOpts).Send() |
There was a problem hiding this comment.
We should always call the non-request method (e.g. SetIdentityNotificationTopic), because it handles some logic for us
|
|
||
| notificationAttributes := response.NotificationAttributes[identity] | ||
| switch notification { | ||
| case "Bounce": |
There was a problem hiding this comment.
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.
| Schema: map[string]*schema.Schema{ | ||
| "topic_arn": &schema.Schema{ | ||
| Type: schema.TypeString, | ||
| Required: false, |
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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!
| }, | ||
|
|
||
| "identity": &schema.Schema{ | ||
| Type: schema.TypeSet, |
There was a problem hiding this comment.
This should be of type
TypeStringrather 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
| }, | ||
|
|
||
| "notification_type": &schema.Schema{ | ||
| Type: schema.TypeSet, |
There was a problem hiding this comment.
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
| NotificationType: aws.String(notification), | ||
| SnsTopic: aws.String(topic), | ||
| } | ||
|
|
There was a problem hiding this comment.
It would be really nice to add some debug here, using something in the idea:
log.Printf("[DEBUG] Setting SES Identity Notification: %#v", setOpts)
| getOpts := &ses.GetIdentityNotificationAttributesInput{ | ||
| Identities: []*string{aws.String(identity)}, | ||
| } | ||
|
|
There was a problem hiding this comment.
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)
| } | ||
|
|
||
| func resourceAwsSesNotificationDelete(d *schema.ResourceData, meta interface{}) error { | ||
| d.Set("topic_arn", nil) |
There was a problem hiding this comment.
To delete it, I think we should call the SetIdentityNotificationTopic method, passing nil / empty for the SNS Topic. Thoughts? :)
| notificationAttributes := response.NotificationAttributes[identity] | ||
| switch notification { | ||
| case "Bounce": | ||
| if err := d.Set("topic", notificationAttributes.BounceTopic); err != nil { |
There was a problem hiding this comment.
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? :)
Ninir
added
the
waiting-response
radeksimko
added
the
size/M
radeksimko
changed the title
|
This PR has been superseded by #2640 and merged into master! The new |
|
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
breathingdust
removed
the
waiting-response
Attempts to resolve #931
Would like some feedback on general style and testing?