Suggestion on nested block definitions #682
Conversation
microsoft-github-policy-service
bot
added
the
Needs: Triage 🔍
MariusStorhaug
changed the title
| length = optional(number) | ||
| }))) | ||
| })) | ||
| default = null |
There was a problem hiding this comment.
For collection values we recommend default is empty map/list/set and set nullable to false
There was a problem hiding this comment.
Hey @matt-FFFFFF, thank you for the feedback. Why is this the recommendation?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 inoptional.
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.
There was a problem hiding this comment.
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.
| name = string | ||
| length = optional(number) | ||
| })) | ||
| } |
There was a problem hiding this comment.
Please add nullable = false
| name = string | ||
| length = optional(number) | ||
| nested_block = optional(object(any)) | ||
| nested_multi_blocks = optional(map(object({ |
There was a problem hiding this comment.
By providing a default empty map can we lose the try() below?
| type = map(object({ | ||
| name = string | ||
| length = optional(number) | ||
| nested_block = optional(object(any)) |
There was a problem hiding this comment.
By providing a default empty map can we lose the try below?
| name = string | ||
| length = optional(number) | ||
| })) | ||
| default = null |
There was a problem hiding this comment.
Again please set empty map and nullable false
| # 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 : {} |
There was a problem hiding this comment.
This can be simplified if the default is empty map
matt-FFFFFF
added
Needs: Author Feedback 👂
Overview/Summary
Suggestions to further define conditions, looping and nested blocks.
Supports the following:
countandfor_eachnullcomparison as creation toogledynamiccoalesceortrywhen setting default values for nullable expressionsenabledormodule_depends_onvariableIn conflict with:
nullable = falsewhen it’s possibleThis PR fixes/adds/changes/removes
Nested blockssub-section to TFNFR7 with suggestion on how to implement loops in different circumstances.Breaking Changes
As part of this Pull Request I have
mainbranch