terraform-aws-modules / terraform-aws-autoscaling

Terraform module to create AWS Auto Scaling resources 🇺🇦
https://registry.terraform.io/modules/terraform-aws-modules/autoscaling/aws
Apache License 2.0
288 stars 552 forks source link

Unable to set `var.instance_refresh` to null #264

Closed Kellen275 closed 2 months ago

Kellen275 commented 2 months ago

Description

I would like to conditionally enable the instance_refresh capability when importing this module into my own.

Versions

Reproduction Code

I'm attempting to do this via the input

locals = {
  refresh = false
  default_instance_refresh = {
    strategy = "Rolling"
    preferences = {
      min_healthy_percentage = 100
      max_healthy_percentage = 110
    }
  }
}

inputs = {
  instance_refresh = local.refresh ? local.default_instance_refresh : null
}

However, this throws an error since this module performs an unchecked length(var.instance_refresh) here: https://github.com/terraform-aws-modules/terraform-aws-autoscaling/blob/master/main.tf#L415

Note that I cannot pass {} instead of null because terraform will throw an expected type mismatch.

Additional Comments

I posed the question to terraform folks and they had some suggestions for how this may be fixed: https://discuss.hashicorp.com/t/conditionally-pass-complex-object/66171

bryantbiggs commented 2 months ago

null is not valid, set an empy map {}

Kellen275 commented 2 months ago

An empty map cannot be used here because it produces a type mismatch with local.default_instance_refresh

Inconsistent conditional result types; The true and false result expressions must have consistent types.
The 'true' value includes object attribute "preferences", which is absent in the 'false' value.
Kellen275 commented 2 months ago

I've been able to get this working by simply adding nullable = false to the variable definition here: https://github.com/terraform-aws-modules/terraform-aws-autoscaling/blob/master/variables.tf#L202-L206

However, this would require bumping the minimum supported version from 1.0 to 1.1: https://developer.hashicorp.com/terraform/language/values/variables#disallowing-null-input-values

bryantbiggs commented 2 months ago

if you want to follow this wrapping approach, you have to work around the limitations of Terraform. Since you have not provided a full reproduction, I'll do my best

locals {
  refresh = true

  instance_refresh = { for k, v in {
    strategy = "Rolling"
    preferences = {
      min_healthy_percentage = 100
      max_healthy_percentage = 110
    }
    } : k => v if local.refresh
  }
}

# I don't know what this is, but its not valid on its own like this
inputs = {
  instance_refresh = local.default_instance_refresh
}
Kellen275 commented 2 months ago

Thank you for this example! This does appear to satisfy my usage :slightly_smiling_face:

bryantbiggs commented 2 months ago

closing for now

github-actions[bot] commented 1 month ago

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.