terraform-aws-modules / terraform-aws-acm

Terraform module to create AWS ACM resources πŸ‡ΊπŸ‡¦
https://registry.terraform.io/modules/terraform-aws-modules/acm/aws
Apache License 2.0
182 stars 229 forks source link

feat!: Added validation_option configuration and upgraded AWS provider to v4 #106

Closed philicious closed 2 years ago

philicious commented 2 years ago

Description

Adds validation_domain option as described in #105

Motivation and Context

The ACM-request option --domain-validation-options ValidationDomain=bar.com,DomainName="foo.bar.com" for modifying the domain used for sending the validation email to, couldnt be used yet as the TF-provider didnt support it. Finally, after only 4yrs being open, the PR got merged and its available in provider 4.12.0

How Has This Been Tested?

antonbabenko commented 2 years ago

@bryantbiggs Should this also be a major release? Sometimes I have mixed feelings and am unsure whether a major release is a right decision when the AWS provider goes up from v2 to v4.

bryantbiggs commented 2 years ago

@antonbabenko ya - I think any provider major version update is automatically a module version update. It forces users to upgrade their provider version. We will potentially have a lot of major version upgrades in modules for this but I guess thats how it goes

philicious commented 2 years ago

@antonbabenko is there anything left for me to finish this up ?

philicious commented 2 years ago

@antonbabenko could you do a release with this please ? Would be really appreciated, thank you πŸ™!

bryantbiggs commented 2 years ago

@philicious can you also bump the required minimum Terraform version to 1.0 since we're taking this as a breaking change

philicious commented 2 years ago

@bryantbiggs will do tomorrow πŸ‘πŸΌ

philicious commented 2 years ago

@bryantbiggs we somehow managed to commit the same change almost at the same time :D

bryantbiggs commented 2 years ago

@philicious should we support this through the separate resource instead of the nested attribute in the certificate resource https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/acm_certificate_validation ?

I don't see anything currently that prefers one way versus the other, but in other modules I know its been preferable to use the standalone resource rather than the nested attribute.

[Edit] Or are these two different things that I am confusing πŸ˜…

philicious commented 2 years ago

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/acm_certificate_validation

@bryantbiggs afaict the acm_certificate_validation is already present in the module https://github.com/terraform-aws-modules/terraform-aws-acm/blob/3e88a7102cb95488b192f57774a24056b4adeb24/main.tf#L53-L59

afaiu it has a different purpose: retrieving the status of validation, not configuring validation method. So imho it should also work for domains that have an overridden domain name

antonbabenko commented 2 years ago

This PR is included in version 4.0.0 :tada:

github-actions[bot] commented 1 year ago

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.