hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.36k stars 9.49k forks source link

Optional arguments in object variable type definition #19898

Closed prehor closed 2 years ago

prehor commented 5 years ago

Current Terraform Version

Terraform v0.12.0-alpha4 (2c36829d3265661d8edbd5014de8090ea7e2a076)

Proposal

I like the object variable type and it would be nice to be able to define optional arguments which can carry null value too, to use:

variable "network_rules" {
  default = null
  type = object({
    bypass = optional(list(string))
    ip_rules = optional(list(string))
    virtual_network_subnet_ids = optional(list(string))
  })
}

resource "azurerm_storage_account" "sa" {
  name = random_string.name.result
  location = var.location
  resource_group_name = var.resource_group_name
  account_replication_type = var.account_replication_type
  account_tier = var.account_tier

  dynamic "network_rules" {
    for_each = var.network_rules == null ? [] : list(var.network_rules)

    content {
      bypass = network_rules.value.bypass
      ip_rules = network_rules.value.ip_rules
      virtual_network_subnet_ids = network_rules.value.virtual_network_subnet_ids
    }
  }

instead of:

variable "network_rules" {
  default = null
  type = map(string)
}

resource "azurerm_storage_account" "sa" {
  name = random_string.name.result
  location = var.location
  resource_group_name = var.resource_group_name
  account_replication_type = var.account_replication_type
  account_tier = var.account_tier

  dynamic "network_rules" {
    for_each = var.network_rules == null ? [] : list(var.network_rules)

    content {
      bypass = lookup(network_rules, "bypass", null) == null ? null : split(",", lookup(network_rules, "bypass"))
      ip_rules = lookup(network_rules, "ip_rules", null) == null ? null : split(",", lookup(network_rules, "ip_rules"))
      virtual_network_subnet_ids = lookup(network_rules, "virtual_network_subnet_ids", null) == null ? null : split(",", lookup(network_rules, "virtual_network_subnet_ids"))
    }
  }
}
Nuru commented 2 years ago

@apparentlymart Can we get an ETA (or declaration of lack of interest in an ETA) for taking this feature out of experimental and into production? I would really like to be using it in a lot of places now. It has been experimental in 0.14, 0.15, 1.0, and 1.1. Can we get it into production at 1.2? CC: @alisdair

Until the experiment is concluded, the behavior of this feature may see breaking changes even in minor releases. We recommend using this feature only in prerelease versions of modules as long as it remains experimental.

apparentlymart commented 2 years ago

Hi all,

Based on feedback about the existing experiment, we don't intend to graduate it to be a stable feature exactly as currently designed. Instead, we intend to make another design iteration which better integrates the idea of default values into the constraints system, and deals with some other concerns that folks raised in feedback on the existing proposal.

We don't intend to take the experiment away until there's a new experiment to replace it, but if you do wish to use it in production in spite of its experimental status I suggest keeping in mind that you will probably need to change your configuration at least a little in response to the changes for the second round of experiment.

The purpose of experiments is to gather feedback before finalizing a design, so in this case the experiment process worked as intended but since the feedback led to us needing another round of design we had to wait until there was time to spend on that design work, since the team was already wholly assigned to other work. We'll share more here when there's more to share.

joe-a-t commented 2 years ago

Flagging https://github.com/hashicorp/terraform/issues/30608 as a related enhancement request that might merit consideration for future state consistency when refactoring of the optional arguments stuff here.

erikeckhardt commented 2 years ago

It seems like long-term, to enforce types in a way that works for reasonable use cases, following in the pattern of TypeScript could be extremely useful. Let's say there are two properties that are mutually exclusive. Even with optional(), there's no way to express with the type system that either ONE or the OTHER must be present—that would have to be a validation error that occurs at runtime instead of giving an error at design time.

Here's a concrete example of a real use-case for creating AWS CloudWatch dashboard widget JSON: properties query and metrics are mutually exclusive, but one is required. Using optional() might look like this:

variable widget_definition {
  type = object({
    metrics = optional(list(list(string)))
    query = optional(string)
    ... other attributes here
  })
  description = "An object similar in structure to the JSON for CloudWatch dashboard widgets, but with some default values and simplifications/helpers to make the input simpler."
  validation {
    condition = count([ for key in var.widget_definition: key if contains(["metrics", "query"], key) ]) == 1
    error_message = "Must provide exactly one of properties 'metrics' and 'query' in variable widget_definition."
  }

But TypeScript handles this with type unions. That might look something like this:

  type = object({
     ... other attributes here
  }) & (
    { metrics = list(list(string) }
    | { query = string }
  )

Using something like this would avoid the need for validation to enforce the mutual exclusivity, allowing the type system itself to represent the strict expected types and the either/or nature of both metrics and query.

fabiendelpierre commented 2 years ago

Looks like this is happening with TF v1.3.0.

https://github.com/hashicorp/terraform/releases/tag/v1.3.0-alpha20220608

korinne commented 2 years ago

@fabiendelpierre You beat me to it :) Hi all, we'd love your feedback on the latest v1.3 alpha, which includes the ability to mark object type attributes as optional and set default values. You can learn more/provide feedback here. Thank you in advance!

prowlaiii commented 2 years ago

Further to my previous post, I would suggest that (though the v1.3.0 adding of the 2nd default field in the "optional()" function is an improvement) the "optional()" keyword/function itself is really a bit of a red-herring and simply expanding the variable structure to allow nesting would be a more natural and syntactically consistent approach.

A simple variable declaration:

variable "name" {
  type    = string
  default = "blah"
}

A complex variable declaration:

variable "settings" {
  type = object({
    name          = string       # shorthand for "{ type = string }"
    description   = { type = string, default = "The name" }
    how_many      = { type = number, default = 1, description = "Count of items" }
    list_of_items = { type = list, default = [] }
  })
}

Just as with normal variables, the act of declaring a default makes it optional; there is no need for an "optional()" keyword/function and conversely not specifying a "default" makes it a required item.

Thinking about it, the "object()" declaration could become redundant too, as the fact that there is a mix of types would implicitly make that so.

variable "settings" {
  type = {
    name          = string       # shorthand for "{ type = string }"
    description   = { type = string, default = "The name" }
    how_many      = { type = number, default = 1, description = "Count of items" }
    list_of_items = { type = list, default = [] }
  }
}

Extrapolating that further, the "type =" lines could be recursive and thus support sub-objects .

atavakoliyext commented 2 years ago

@prowlaiii is what you're describing similar to the feature request in #22449?

prowlaiii commented 2 years ago

@prowlaiii is what you're describing similar to the feature request in #22449?

I think it is indeed; I hadn't seen it, but I knew I couldn't have been the first person on the planet to think of it!

(I'm also suggesting the "description =" item and removing the need for "object()" there.)

FYI, I also raised this in the v1.3 feedback and have now included a link to yours there. https://discuss.hashicorp.com/t/request-for-feedback-optional-object-type-attributes-with-defaults-in-v1-3-alpha/40550/47

dylanturn commented 2 years ago

A complex variable declaration:

variable "settings" {
  type = object({
    name          = string       # shorthand for "{ type = string }"
    description   = { type = string, default = "The name" }
    how_many      = { type = number, default = 1, description = "Count of items" }
    list_of_items = { type = list, default = [] }
  })
}

If we're going to take it that far then why not just go all the way and create a new type that lets you simply specify an openapi schema?

The openapi shema variable type could be defined inline, sourced from a file in the same directory, or maybe even from a remote source (like we source modules today). That capability would make it easy to create, document, and reuse complex data classes in our input and output blocks.

apparentlymart commented 2 years ago

Hi all,

An evolution of the optional attributes experiment, with built-in support for default values and no separate defaults function, is going to graduate to stable in the forthcoming v1.3.0 release.

For that reason, I'm going to close this issue to represent that the base use-case is met. I see that there is some discussion here about further additions to the model for object attributes, but I think it'll be better to discuss those in more specific issues once the initial features are included in a stable release and we can learn from experiences of using the first iteration of this functionality in a broad set of Terraform modules with differing design needs.

Thanks for the great discussion here!

github-actions[bot] commented 1 year 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.