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

Default "zone_id" to empty string for "dns" module #135

Closed
wants to merge 1 commit into from

Conversation

marcuz
Copy link
Contributor

@marcuz marcuz commented Oct 25, 2021

what

  • zone_id passed to dns module must be a string

why

  • dns module expects zone_id to be type string
  • If var.zone_id[0] try function call fails it defaults to [], which terraform complains about:
│ Error: Invalid value for module argument
│ 
│   on .terraform/modules/[REDACTED]/main.tf line 199, in module "dns":
│  199:   zone_id  = try(var.zone_id[0], var.zone_id)
│ 
│ The given value is not suitable for child module variable "zone_id" defined at .terraform/modules/[REDACTED].dns/variables.tf:1,1-19: string required.

I am unsure why it doesn't simply skip this check because of length(var.zone_id) > 0 (which is []). 🤔

My current workaround is setting zone_id = "" in the module resource.

references

@marcuz marcuz requested review from a team as code owners October 25, 2021 09:56
@marcuz marcuz requested review from nitrocode and woz5999 and removed request for a team October 25, 2021 09:56
@nitrocode
Copy link
Member

nitrocode commented Oct 25, 2021

It looks like the change in PR #133 was due to closing #82

The downstream https://github.com/cloudposse/terraform-aws-route53-cluster-hostname#input_zone_id requires a string and in this module, zone_id is using type any.

The issue with your PR is that it will skip inserting the var.zone_id unless it's a list.

cc: @Nuru , Could we simply change the variable default to null to get around this ?

or we could upgrade the https://github.com/cloudposse/terraform-aws-route53-cluster-hostname to take an input of a list

Nuru added a commit that referenced this pull request Oct 25, 2021
@Nuru Nuru mentioned this pull request Oct 25, 2021
@Nuru
Copy link
Sponsor Contributor

Nuru commented Oct 25, 2021

@nitrocode Fixing route53-cluster-hostname to take a list is the right long-term solution, but we don't need to wait for that.

@Nuru Nuru closed this in #136 Oct 25, 2021
Nuru added a commit that referenced this pull request Oct 25, 2021
* Fix #134 use only existing security groups

* Fix #135, default zone ID
@marcuz marcuz deleted the patch-1 branch October 26, 2021 05:30
brian-weis-msr pushed a commit to Measurabl/terraform-aws-elasticache-redis that referenced this pull request Apr 2, 2024
* Fix cloudposse#134 use only existing security groups

* Fix cloudposse#135, default zone ID
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants