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.

Sign up for GitHub

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

Jump to bottom

fix: Correct validation_option variable type #112

Merged
merged 1 commit into from
Jun 17, 2022
Merged

fix: Correct validation_option variable type #112

merged 1 commit into from
Jun 17, 2022

Conversation

philicious
Copy link
Contributor

@philicious philicious commented Jun 17, 2022 •
edited
Loading

Description

in #106 we managed to merge non-working code, I am afraid. A suggestion by @bryantbiggs changed the map(object..) to a map(string) which I applied. however it needs to be a map(any)
It probably went unnoticed when @antonbabenko tested the example as he used a version that hadnt the suggestion applied yet.

Motivation and Context

using v4.0 results in

module "acm" {
  source  = "terraform-aws-modules/acm/aws"
  version = "~> v4.0"

  validation_option = {
    "default" = {
      domain_name = var.domain_name
      validation_domain = var.validation_domain
    }
  }
The given value is not suitable for child module variable "validation_option" defined at .terraform/modules/dns.acm/variables.tf:54,1-29: element "default": string required.

when changing the variable type, TF shows a diff as expected

bryantbiggs
bryantbiggs reviewed Jun 17, 2022
View reviewed changes
variables.tf Outdated
@@ -53,7 +53,7 @@ variable "validation_method" {

variable "validation_option" {
description = "The domain name that you want ACM to use to send you validation emails. This domain name is the suffix of the email addresses that you want ACM to use."
type = map(string)
type = map(any)
Copy link
Member

Choose a reason for hiding this comment

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

ugh, these types are more pain that value currently. lets just go with any for now till we get to v1.3 support

Suggested change
type = map(any)
type = any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bryantbiggs updated vars and README. ( even map(map(string)) would have worked as I tested. but that looked to ugly to me *G)

Copy link
Member

Choose a reason for hiding this comment

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

ya, its a shame - but once we reach a point where we can set v1.3 as minimum we can have better defined variable data structures. Thanks for the fix!

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can/should consider upgrading to 1.3 earlier than at least 6-10 months after it will be released. For now, any is the most flexible way + we have the examples to see the structure.

@philicious philicious force-pushed the fix-validation-option branch from 440f915 to 321ef3b Compare June 17, 2022 18:52
@philicious philicious force-pushed the fix-validation-option branch from 321ef3b to 1db86f4 Compare June 17, 2022 18:53
@bryantbiggs bryantbiggs changed the title fix: Type of var.validation_option fix: Correct validation_option variable type Jun 17, 2022
@bryantbiggs bryantbiggs merged commit 69c1f88 into terraform-aws-modules:master Jun 17, 2022
antonbabenko pushed a commit that referenced this pull request Jun 17, 2022
@semantic-release-bot
chore(release): version 4.0.1 [skip ci]
1df1c85 
### [4.0.1](v4.0.0...v4.0.1) (2022-06-17)

### Bug Fixes

* Type of var.validation_option ([#112](#112)) ([69c1f88](69c1f88))
@antonbabenko
Copy link
Member

This PR is included in version 4.0.1 🎉

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

@antonbabenko antonbabenko antonbabenko left review comments

@bryantbiggs bryantbiggs bryantbiggs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@philicious @antonbabenko @bryantbiggs