Add AWS SES DKIM resource #1786
Conversation
radeksimko
added
enhancement
There was a problem hiding this comment.
@arr-dev, thank you so much for uplifting this to the new provider location! 🙇
That said: This isn't working for me in the current version, but I got it working locally, so comments inbound...
aws/resource_aws_ses_domain_dkim.go
Outdated
| Delete: resourceAwsSesDomainDkimDelete, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "arn": { |
There was a problem hiding this comment.
I am not sure what the arn field is used for in this resource? I didn't find any references to constructing an ARN for dkim/ as you do below, and I'm not sure where this is used. I was able to get my local use of this resource working fine even after removing the all arn-related code.
There was a problem hiding this comment.
I've based this resource on aws_ses_domain_identity which computes arn the same way.
I understand that it's not used, but I wasn't sure about leaving it out.
I'll remove it.
aws/resource_aws_ses_domain_dkim.go
Outdated
| Create: resourceAwsSesDomainDkimCreate, | ||
| Read: resourceAwsSesDomainDkimRead, | ||
| Delete: resourceAwsSesDomainDkimDelete, | ||
|
|
There was a problem hiding this comment.
Can we add an Importer here so importing works?
Importer: &schema.ResourceImporter{
State: schema.ImportStatePassthrough,
},
The above worked for me.
aws/resource_aws_ses_domain_dkim.go
Outdated
| } | ||
|
|
||
| d.Set("arn", fmt.Sprintf("arn:%s:ses:%s:%s:dkim/%s", meta.(*AWSClient).partition, meta.(*AWSClient).region, meta.(*AWSClient).accountid, d.Id())) | ||
| d.Set("dkim_tokens", verificationAttrs.DkimTokens) |
There was a problem hiding this comment.
Most important: this resource did not work with this line as written!
Without changing this, the tokens weren't actually written to the state file under dkim_tokens, and the next plan attempted to update the resource, which isn't possible, so Terraform errored out.
I needed to wrap the DkimTokens in a aws.StringValueSlice to get the []*string converted to []string, as (sort of) documented here: https://godoc.org/github.com/aws/aws-sdk-go/aws
d.Set("dkim_tokens", aws.StringValueSlice(verificationAttrs.DkimTokens))
Add Importer Remove `arn` attribute
|
@arr-dev Thank you so much for the changes! I'll test them live in a bit, but my eyeball read-over makes me think they will work just fine. :-) In my perfect world, I'd love to see the documentation updated to mention that |
|
(And FTR, I'm an outside contributor just like you, so I don't have any merge powers; we'll still need to wait for a Terraform project committer to approve and merge...) |
|
OK, confirmed working tested locally! Whoop, thank you! |
|
@handlerbot Yes, you are right and I'd be glad to complete this PR. |
|
I've added a commit with doc fixes,
Not sure if I'm running it correctly. |
|
Managed to run Acceptance tests, so I've added tests which verify tokens state. If anyone needs it, command to run specific acceptance test: |
|
@radeksimko This looks ready to go from a "I tested this in the field" perspective, can we get this looked at? |
There was a problem hiding this comment.
@arr-dev Thanks for the PR
@handlerbot Thanks for the peer review and ping.
Yeey, teamwork 💯
|
Teamwork makes the dream work, as they say. :-D |
|
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
Added aws_ses_dkim resource, according to http://docs.aws.amazon.com/ses/latest/DeveloperGuide/easy-dkim-dns-records.html
Transfered the PR from hashicorp/terraform#14831
Closes #1347