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

Suggestion on nested block definitions #682

Closed
wants to merge 13 commits into from
41 changes: 30 additions & 11 deletions docs/content/specs/terraform/_index.md
Original file line number Diff line number Diff line change
Expand Up @@ -345,13 +345,13 @@ Meta-arguments, arguments and nested blocked are separated by blank lines.

```terraform
dynamic "linux_profile" {
for_each = var.admin_username == null ? [] : ["linux_profile"]
for_each = var.linux_profile != null ? { this = var.linux_profile } : {}

content {
admin_username = var.admin_username
admin_username = linux_profile.value.admin_username

ssh_key {
key_data = replace(coalesce(var.public_ssh_key, tls_private_key.ssh[0].public_key_openssh), "\n", "")
key_data = linux_profile.value.ssh_key
}
}
}
Expand Down Expand Up @@ -500,28 +500,47 @@ Please use this technique under this use case only.

#### ID: TFNFR12 - Category: Code Style - Optional nested object argument should use `dynamic`

An example from the community:
An example from the interfaces for managed_identity:

```terraform
resource "azurerm_kubernetes_cluster" "main" {
...
dynamic "identity" {
for_each = var.client_id == "" || var.client_secret == "" ? [1] : []
for_each = local.managed_identities.user_assigned

content {
type = var.identity_type
user_assigned_identity_id = var.user_assigned_identity_id
type = identity.value.type
identity_ids = identity.value.user_assigned_resource_ids
}
}
...
}
```

Please refer to the coding style in the example. If you just want to declare some nested block under conditions, please use:
Resources nested blocks are implemented differently depending on if they are optional or required and support one or more blocks. These **SHOULD** be implemented in the following ways:

{{< /hint >}}

{{< tabs "nested blocks" >}}
{{< tab "Required, one" >}}
{{< include file="/static/includes/spec/int.spec.nest.req.one.tf" language="terraform" options="linenos=false" >}}
{{< /tab >}}
{{< tab "Required, one or more" >}}
{{< include file="/static/includes/spec/int.spec.nest.req.more.tf" language="terraform" options="linenos=false" >}}
{{< /tab >}}
{{< tab "Optional, one" >}}
{{< include file="/static/includes/spec/int.spec.nest.opt.one.tf" language="terraform" options="linenos=false" >}}
{{< /tab >}}
{{< tab "Optional, one or more" >}}
{{< include file="/static/includes/spec/int.spec.nest.opt.more.tf" language="terraform" options="linenos=false" >}}
{{< /tab >}}
{{< tab "Nested dynamic blocks" >}}
{{< include file="/static/includes/spec/int.spec.nest.nested.tf" language="terraform" options="linenos=false" >}}
{{< /tab >}}
{{< /tabs >}}

{{< hint type=note >}}

```terraform
for_each = <condition> ? [<some_item>] : []
```

<br>

Expand Down
46 changes: 46 additions & 0 deletions docs/static/includes/specification/int.spec.nest.nested.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# The resource has nested dynamic blocks

# Variable declaration
variable "optional_multi_block" {
type = map(object({
name = string
length = optional(number)
nested_block = optional(object(any))
Copy link
Member

Choose a reason for hiding this comment

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

By providing a default empty map can we lose the try below?

nested_multi_blocks = optional(map(object({
Copy link
Member

@matt-FFFFFF matt-FFFFFF Feb 28, 2024

Choose a reason for hiding this comment

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

By providing a default empty map can we lose the try() below?

name = string
length = optional(number)
})))
}))
default = null
Copy link
Member

Choose a reason for hiding this comment

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

For collection values we recommend default is empty map/list/set and set nullable to false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @matt-FFFFFF, thank you for the feedback. Why is this the recommendation?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @MariusStorhaug,

We prefer this because then the author does not need to check for a null value before using the variable in an expression. It is either empty, or it is not.

The only time we recommend otherwise is if there is a semantic difference between null and an empty collection.

In most resource modules that are authoritative about the resources they deploy, nullable = false is the correct approach.

Copy link
Contributor Author

@MariusStorhaug MariusStorhaug Mar 13, 2024

Choose a reason for hiding this comment

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

I was instead thinking of optimizing for the 'pattern creator' or a direct consumer of the resource module. To me it makes more sense having checks in the code and have a clear interface where we support both null and empty ([], {}). That would create a clear definition where:

  • null - don't use the feature
  • empty ([], {}) - use the default values in optional.

This would help when defining variables in patterns that would map to the modules that a pattern would consume and you want full flexibility to the consumed resource module.

Copy link
Member

Choose a reason for hiding this comment

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

As I said before this very much depends on whether there is a semantic meaning to null vs {}.

In the case of a map being used in a for_each expression, there is not. Therefore it makes sense to simplify the authoring experience and set nullable to false.

In the case where we are defining an absolute list of something, and the difference between null and {} would mean that supplying an empty collection would affect existing resources, then of course we should make a sensible decision. However in this case map() may not be the correct type to use.

I still believe that for 80% of the cases that nullable = false is correct for most top level collection values. Of course there are always exceptions.

}

# Resource declaration
resource "my_resource" "this" {

dynamic "optional_multi_block" {
for_each = var.optional_multi_block != null ? var.optional_multi_block : {}

content {
name = var.optional_multi_block.name
length = var.optional_multi_block.length

dynamic "nested_block" {
for_each = try(optional_multi_block.value.nested_block, null) != null ? { this = var.optional_multi_block.value.nested_block } : {}

content {
name = var.optional_multi_block.value.nested_block.name
length = var.optional_multi_block.value.nested_block.length
}
}

dynamic "nested_multi_blocks" {
for_each = try(optional_multi_block.value.nested_multi_blocks, null) != null ? var.optional_multi_block.value.nested_multi_blocks : {}

content {
name = nested_multi_blocks.value.name
length = nested_multi_blocks.value.length
}
}
}
}
}
25 changes: 25 additions & 0 deletions docs/static/includes/specification/int.spec.nest.opt.more.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# The resource supports one or more optional nested blocks

# Variable declaration
# Default value is null, as it is optional and should not configure anything by default.
# Uses a map type to support multiple instances.
variable "optional_multi_block" {
type = map(object({
name = string
length = optional(number)
}))
default = null
Copy link
Member

Choose a reason for hiding this comment

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

Again please set empty map and nullable false

}

# Resource declaration
# The dynamic block is used to support the optionality and looping.
resource "my_resource" "this" {
dynamic "optional_multi_block" {
for_each = var.optional_multi_block != null ? var.optional_multi_block : {}
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified if the default is empty map


content {
name = optional_multi_block.value.name
length = optional_multi_block.value.length
}
}
}
26 changes: 26 additions & 0 deletions docs/static/includes/specification/int.spec.nest.opt.one.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# The resource supports a single optional nested block

# Variable declaration
# Default value is null, as it is optional and should not configure anything by default.
# It uses an object type to support a single instance.
variable "optional_single_block" {
type = object({
name = string
length = optional(number)
})
default = null
}

# Resource declaration
# The dynamic block provides the optionality.
# The `this` key is used to access the current instance and maps the content to the name of the block.
resource "my_resource" "this" {
dynamic "optional_single_block" {
for_each = var.optional_single_block != null ? { this = var.optional_single_block } : {}

content {
name = optional_single_block.value.name
length = optional_single_block.value.length
}
}
}
24 changes: 24 additions & 0 deletions docs/static/includes/specification/int.spec.nest.req.more.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# The resource requires one or more nested blocks

# Variable declaration
# No default value, as it is required in the module.
# Uses a map type to support multiple instances.
variable "required_multi_blocks" {
type = map(object({
name = string
length = optional(number)
}))
}
Copy link
Member

Choose a reason for hiding this comment

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

Please add nullable = false


# Resource declaration
# The dynamic block is used, as there are multiple instances.
resource "my_resource" "this" {
dynamic "required_multi_blocks" {
for_each = var.required_multi_blocks

content {
name = required_multi_blocks.value.name
length = required_multi_blocks.value.length
}
}
}
21 changes: 21 additions & 0 deletions docs/static/includes/specification/int.spec.nest.req.one.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# The resource requires a single nested block

# Variable declaration
# No default value, as it is required in the module.
# Uses an object type to support a single instance.
variable "required_single_block" {
type = object({
name = string
length = optional(number)
})
}

# Resource declaration
# The block is used directly, as only one instance is supported and required.
resource "my_resource" "this" {

required_single_block {
name = var.required_single_block.name
length = var.required_single_block.length
}
}