provider/aws: Implement aws_ses_domain_identity #13098

dougneal · 2017-03-27T16:03:23Z

Adding the capability to add domain identities to Amazon SES and capture the domain verification code.

Tasks:

Implement CRUD functions
Implement import
Complete test coverage
Update documentation

Should (partially) satisfy #11232.

dougneal · 2017-03-28T10:26:10Z

Build failure appears to be caused by unrelated code.

radeksimko

Other than randomization of the test, this looks pretty good to me and it's a great start for SES identities! 👍

Let me know if you need any help with addressing the mentioned point(s) or if you have any other questions regarding this.

radeksimko · 2017-03-30T12:28:06Z

builtin/providers/aws/resource_aws_ses_domain_identity_test.go

+
+const testAccAwsSESDomainIdentityConfig = `
+resource "aws_ses_domain_identity" "test" {
+	domain = "example.com"


Would you mind randomizing this domain, so that we can safely run this test multiple times in parallel?
Have a look at how we do it in other tests, e.g. here

I think for the purpose of this test it doesn't matter if we're ever going to be able to finish the verification process (i.e. own the domain or manage DNS for it), so I'd go for something like %d.terraformtesting.com.

radeksimko · 2017-03-30T12:29:49Z

builtin/providers/aws/resource_aws_ses_domain_identity.go

+	}
+
+	verificationAttrs := response.VerificationAttributes[domainName]
+	if verificationAttrs == nil {


I think more idiomatic way to check the existence of a key in a map would be something like

verificationAttrs, ok := response.VerificationAttributes[domainName] if !ok {

radeksimko · 2017-03-30T12:34:36Z

website/source/docs/providers/aws/r/ses_domain_identity.html.markdown

+
+# aws\_ses\_domain_identity
+
+Provides an SES domain identity resource


Would you mind attaching some links and/or details about what steps are expected from the user to actually finish the verification of the domain? What do you think about adding a Route53 DNS record to the example + mentioning the WHOIS records need to point to the zone, or eventually delegate to it? It may sound like an obvious thing, but I think it's good to explain to what extent do we automate this process and help the user along the way.

http://docs.aws.amazon.com/ses/latest/DeveloperGuide/dns-txt-records.html

radeksimko · 2017-03-30T12:36:36Z

website/source/docs/providers/aws/r/ses_domain_identity.html.markdown

+  will signal to SES that the owner of the domain has authorised SES to act on
+  their behalf. The domain identity will be in state "verification pending"
+  until this is done. Find out more about verifying domains in Amazon SES in
+  the [AWS SES docs](http://docs.aws.amazon.com/ses/latest/DeveloperGuide/verify-domains.html).


Oh, I see, you have the explanation here actually, that's good 👍 but I still think we should include an example of a DNS record, like Route53 record.

dougneal · 2017-03-30T13:26:46Z

Thanks for the feedback @radeksimko, I've addressed all points. I reordered the documentation slightly to put the example at the end including a Route 53 resource to add the verification token.

radeksimko

Thanks for the updates and sorry I missed the two things in the last review. I hope it's understandable & not too difficult to fix 🙈

Let me know if you want me to do it instead, post-merge.

radeksimko · 2017-03-31T07:39:42Z

builtin/providers/aws/resource_aws_ses_domain_identity_test.go

+		_, ok := err.(awserr.Error)
+		if !ok {
+			return err
+		}


Probably a nitpick, but these 4 line of code are redundant since we always return from any error above (which is the right thing to do btw. as there's nothing like NotFound error in SES API). Would you mind removing these - just to avoid confusions?

Sure. I actually lifted most of the code from here so there is probably some further cleanup to be done across the SES code...

Good catch, I think that is slightly different situation as you may get errors, but we should be scoping it down to specific error code(s): http://docs.aws.amazon.com/ses/latest/APIReference/API_DescribeReceiptRule.html#API_DescribeReceiptRule_Errors

radeksimko · 2017-03-31T07:44:13Z

builtin/providers/aws/resource_aws_ses_domain_identity.go

+		return nil
+	}
+
+	d.Set("verification_token", *verificationAttrs.VerificationToken)


I'm not sure why I didn't notice this one before 🙀 , but we tend to avoid pointer dereferencing in Set() - the function has a built-in functionality to do this safely for all primitive data types and we have been bitten many times in the past by doing this in the resource code.

The API (for any weird reason) may not return the token here, in which case we'd be dereferencing pointer to nil which would in turn cause crash.

Cool, so should I just pass the pointer in?

Provide a resource to manage domain identities in SES. Exports the verification_code attribute which can be used to add the TXT record to the domain to complete the domain verification.

Provide documentation for the new resource type.

dougneal · 2017-03-31T12:35:31Z

All done!

ghost · 2020-04-14T02:21:23Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

dougneal force-pushed the enh-aws_ses_domain_identity branch 2 times, most recently from ce287f3 to 6997ddc Compare March 27, 2017 21:18

dougneal changed the title ~~[WIP] provider/aws: Implement aws_ses_domain_identity~~ provider/aws: Implement aws_ses_domain_identity Mar 27, 2017

dougneal force-pushed the enh-aws_ses_domain_identity branch from 6997ddc to 9852daf Compare March 27, 2017 22:31

radeksimko reviewed Mar 30, 2017

View reviewed changes

radeksimko added new-resource provider/aws waiting-response An issue/pull request is waiting for a response from the community labels Mar 30, 2017

dougneal force-pushed the enh-aws_ses_domain_identity branch from 9852daf to 43dc3d4 Compare March 30, 2017 13:17

radeksimko reviewed Mar 31, 2017

View reviewed changes

dougneal force-pushed the enh-aws_ses_domain_identity branch from 43dc3d4 to de30c25 Compare March 31, 2017 11:50

dougneal added 3 commits March 31, 2017 12:51

provider/aws: New resource: aws_ses_domain_identity

9233db7

Provide a resource to manage domain identities in SES. Exports the verification_code attribute which can be used to add the TXT record to the domain to complete the domain verification.

provider/aws: Acceptance tests for aws_ses_domain_identity

5d0e6a1

Resource aws_ses_domain_identity: Documentation update

68f9884

Provide documentation for the new resource type.

dougneal force-pushed the enh-aws_ses_domain_identity branch from de30c25 to 68f9884 Compare March 31, 2017 11:51

radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Apr 1, 2017

radeksimko approved these changes Apr 1, 2017

View reviewed changes

radeksimko merged commit 0c4c578 into hashicorp:master Apr 1, 2017

dougneal mentioned this pull request Apr 4, 2017

Add keyword: aws_ses_domain_identity hashivim/vim-terraform#27

Merged

ghost locked and limited conversation to collaborators Apr 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Implement aws_ses_domain_identity #13098

provider/aws: Implement aws_ses_domain_identity #13098

dougneal commented Mar 27, 2017 •

edited

Loading

dougneal commented Mar 28, 2017

radeksimko left a comment

radeksimko Mar 30, 2017

radeksimko Mar 30, 2017

radeksimko Mar 30, 2017

radeksimko Mar 30, 2017

dougneal commented Mar 30, 2017

radeksimko left a comment •

edited

Loading

radeksimko Mar 31, 2017

dougneal Mar 31, 2017 •

edited

Loading

radeksimko Apr 1, 2017

radeksimko Mar 31, 2017

dougneal Mar 31, 2017

dougneal commented Mar 31, 2017

ghost commented Apr 14, 2020


		# aws\_ses\_domain_identity

		Provides an SES domain identity resource

provider/aws: Implement aws_ses_domain_identity #13098

provider/aws: Implement aws_ses_domain_identity #13098

Conversation

dougneal commented Mar 27, 2017 • edited Loading

dougneal commented Mar 28, 2017

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougneal commented Mar 30, 2017

radeksimko left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougneal Mar 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougneal commented Mar 31, 2017

ghost commented Apr 14, 2020

dougneal commented Mar 27, 2017 •

edited

Loading

radeksimko left a comment •

edited

Loading

dougneal Mar 31, 2017 •

edited

Loading