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
289 stars 553 forks source link

fix: use try for instance_requirements to avoid null error #223

Closed nitrocode closed 1 year ago

nitrocode commented 1 year ago

Description

Motivation and Context

The issue is when we try to create logic to either use the instance_type or use instance_requirements

module "default" {
  source  = "terraform-aws-modules/autoscaling/aws"
  version = "6.7.1"

  # ...

  instance_requirements = var.instance_type == null ? {
    accelerator_count = {
      max = var.accelerator_count
    }

    instance_generations    = ["current"]

    # ...
  } : null

The null cannot be changed to {} or it will trigger an error

If the instance_type is left as null then it will cause an issue when trying to do the length(null) which is why I am proposing the try()

Breaking Changes

None

How Has This Been Tested?

bryantbiggs commented 1 year ago

I don't think this is the appropriate way to solve this. If you want to go this route, what you have shown with wrapping your conditional, I would move the conditional check inside the map to avoid this mis-matched types on a conditional

nitrocode commented 1 year ago

Hmm how would you do that? :thinking:

bryantbiggs commented 1 year ago
  instance_requirements = { for k,v in { 
    accelerator_count = {
      max = var.accelerator_count
    }
    instance_generations    = ["current"]
  }: k => v if var.instance_type == null }
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.