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

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

Merged

Conversation

philicious
Copy link
Contributor

@philicious philicious commented Apr 30, 2022

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?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@philicious philicious force-pushed the validation-option branch 2 times, most recently from af6695f to 74ffc67 Compare April 30, 2022 15:53
@antonbabenko
Copy link
Member

@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.

@philicious philicious force-pushed the validation-option branch 5 times, most recently from 9584445 to d02c7b0 Compare April 30, 2022 18:33
@bryantbiggs
Copy link
Member

@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
Copy link
Contributor Author

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

@philicious
Copy link
Contributor Author

@antonbabenko could you do a release with this please ?
Would be really appreciated, thank you 🙏!

@bryantbiggs
Copy link
Member

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

@philicious
Copy link
Contributor Author

@bryantbiggs will do tomorrow 👍🏼

@philicious philicious force-pushed the validation-option branch 2 times, most recently from 1c29f64 to 003f13b Compare June 15, 2022 12:21
@philicious
Copy link
Contributor Author

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

main.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@bryantbiggs
Copy link
Member

bryantbiggs commented Jun 15, 2022

@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
Copy link
Contributor Author

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

resource "aws_acm_certificate_validation" "this" {
count = local.create_certificate && var.validation_method != "NONE" && var.validate_certificate && var.wait_for_validation ? 1 : 0
certificate_arn = aws_acm_certificate.this[0].arn
validation_record_fqdns = flatten([aws_route53_record.validation.*.fqdn, var.validation_record_fqdns])
}

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 antonbabenko changed the title feat: Added validation_option configuration feat!: Added validation_option configuration and upgraded AWS provider to v4 Jun 15, 2022
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, if @philicious can confirm that you ran code in the examples/complete-email-validation-with-validation-domain and that it works as expected.

@philicious philicious force-pushed the validation-option branch 2 times, most recently from 590be31 to 03e0ff4 Compare June 15, 2022 14:46
@philicious philicious force-pushed the validation-option branch 2 times, most recently from 0634575 to 9af364d Compare June 15, 2022 14:58
@antonbabenko antonbabenko merged commit 0825e18 into terraform-aws-modules:master Jun 15, 2022
antonbabenko pushed a commit that referenced this pull request Jun 15, 2022
## [4.0.0](v3.5.0...v4.0.0) (2022-06-15)

### ⚠ BREAKING CHANGES

* Added validation_option configuration and upgraded AWS provider to v4 (#106)

### Features

* Added validation_option configuration and upgraded AWS provider to v4 ([#106](#106)) ([0825e18](0825e18))
@antonbabenko
Copy link
Member

This PR is included in version 4.0.0 🎉

@philicious philicious deleted the validation-option branch June 16, 2022 16:41
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants