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

Update aws_acm_certificate to add custom domain for email verification #4662

Closed
wants to merge 5 commits into from
Closed

Conversation

jw-maynard
Copy link

Fixes #3851

Changes proposed in this pull request:

  • Added Optional Arg email_validation_domain
  • Updated creation to populate ValidationDomain with email_validation_domain arg

The code currently uses the same domain for the base domain name and all SANs on the certificate. This is probably the most common use case and avoids having to muck around in the existing domain_validation_options. I realize this breaks with the structure of the AWS API but I don't have an immediate need for the added complexity.

Output from acceptance testing:


make testacc TEST=./aws TESTARGS='-run=TestAccAWSAcmCertificate_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAcmCertificate_ -timeout 120m
=== RUN   TestAccAWSAcmCertificate_emailValidation
--- PASS: TestAccAWSAcmCertificate_emailValidation (23.89s)
=== RUN   TestAccAWSAcmCertificate_dnsValidation
--- PASS: TestAccAWSAcmCertificate_dnsValidation (21.02s)
=== RUN   TestAccAWSAcmCertificate_root
--- PASS: TestAccAWSAcmCertificate_root (23.17s)
=== RUN   TestAccAWSAcmCertificate_rootAndWildcardSan
--- PASS: TestAccAWSAcmCertificate_rootAndWildcardSan (25.57s)
=== RUN   TestAccAWSAcmCertificate_san_single
--- PASS: TestAccAWSAcmCertificate_san_single (24.77s)
=== RUN   TestAccAWSAcmCertificate_san_multiple
--- PASS: TestAccAWSAcmCertificate_san_multiple (24.98s)
=== RUN   TestAccAWSAcmCertificate_wildcard
--- PASS: TestAccAWSAcmCertificate_wildcard (23.04s)
=== RUN   TestAccAWSAcmCertificate_wildcardAndRootSan
--- PASS: TestAccAWSAcmCertificate_wildcardAndRootSan (20.47s)
=== RUN   TestAccAWSAcmCertificate_tags
--- PASS: TestAccAWSAcmCertificate_tags (73.17s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       260.110s

...

@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label May 25, 2018
@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. service/acm Issues and PRs that pertain to the acm service. labels May 25, 2018
@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Jun 5, 2018
@jw-maynard
Copy link
Author

@bflad Wondering when/if this will be merged anytime soon. Just curious if there are any changes that need to be made that are preventing you guys from wanting to merge this or if you've just been too busy else where. Nice work on getting EKS out on day 0 BTW.

@bflad
Copy link
Contributor

bflad commented Jun 6, 2018

Sorry I have not personally had time to give a code review here. At quick glance it seems like we should handle this within the domain validation options so it can handle customizing it for each domain rather than introduce a singular argument which would most likely need to be deprecated in the future.

Turns out there's a previous pull request in this regard: #3853 -- I had some initial PR feedback there since it broke backwards compatibility, but maybe its worth working the original author there, providing feedback, or providing fixes on top of their commits? I'm not sure when I'll personally be able to circle back around to this topic though.

@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Jul 17, 2018
@aeschright aeschright requested a review from a team June 25, 2019 21:24
@bflad
Copy link
Contributor

bflad commented May 27, 2020

Hi @jw-maynard 👋 Thank you again for submitting this. Circling back, we are preferring to focus our efforts on the previous #3853 as part of our spiked effort against the aws_acm_certificate resource in #13053. We are hoping that by the release of version 3.0.0 of the Terraform AWS Provider, we will support this functionality in some form.

@bflad bflad closed this May 27, 2020
@jw-maynard
Copy link
Author

@bflad No problem. We very quickly switched to domain validation after this PR and so I never got around to trying to fix it up. Good luck on version 3.0!

@ghost
Copy link

ghost commented Jun 26, 2020

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 ghost locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/acm Issues and PRs that pertain to the acm service. size/S Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add domain validation options parameter to aws_acm_certificate
2 participants