-
Notifications
You must be signed in to change notification settings - Fork 765
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
Direct support for conditional single-instance resources #1306
Comments
I've marked this as needs-rfc as it requires some pretty careful consideration around how this could be designed. hashicorp/terraform#21953 (comment) is a good summary of the current state of discussion. I see this as two distinct discussions that are linked. One is to add the enabled flag to resources/modules/etc... and handle the result of type.name.attr being an invalid reference if disabled. The other is syntax sugar around accessing these optional values. On initial reading, I'm leaning toward setting a disabled resource/module/etc... to a typed unknown and making sure that references behave as expected, using expressions like I'd recommend that any discussion of "syntax sugar" like the |
Hi @cam72cam, Thanks for spending time on this and being willing to reopen discussion. I agree an RFC process makes sense. I also agree that there are potentially multiple stages in this. For my own use, I haven't ever seen a need for introspection on values that aren't defined. I can usually use the same flag that controls instantiation to control behaviour elsewhere. But maybe others see more benefit from this. On implementation, I'd always favour outcomes that are equivalent to the resource block being absent from the input, so literally not defined. I don't know if this is practical from the code base and I can imagine it's not ideal for the graph traversal. At the same time, to avoid creating downstream dependencies in providers, I wouldn't expect them to be aware of the culling process. Functionally, I'd imagine it would be similar to omitting a value from a for_each block, reducing a count or excluding a resource completely. If we take the for_each case, this is exactly the behaviour we'd expect: a fully specified resource is no longer present. Coming at it from a completely different angle, perhaps there's value in revisiting how singleton resources are represented in the state such that a singleton resource and a resource with a count of one are pseudonyms for the same state? I'm just thinking aloud 😁 Thanks again and I look forward to further comments from the community. |
I think part of what was not explained well in the original issue is that the evaluator needs to know all of the types / existence in an expression in order for it to be valid. In the example: resource "foo" "bar" {
enabled = local.isenabled
}
output "myout" {
value = local.isenabled ? foo.bar.attribute : null
} In that example, we need to know that foo.bar.attribute could exist and is of a certain type. HCL does not do any short circuiting currently, even for && and || operators. These sort of implementation details muddied the waters of the original issue. We need to remember that HCL is a declarative config language that is being shoe-horned into being something close to a programming language. |
Yes, that makes sense. What about if that expression was disallowed but instead a new HCL function was invented that allowed a dynamic lookup but required the type to be stated? When I have to solve similar expressions, I sometimes use a contrived coalesce expression to workaround these cases. The coalesce default value presents a known and typed value that is only present in the case that the resource is absent. Could that approach work? |
I think it's as simple as making foo.bar a "typed unknown" or null value. We know that foo.bar could be a "foo resource" or it might not exist. We don't care if it exists or not until we actually try to access the value. Knowing that it might have a Again, this is getting into the weeds of the OpenTofu and HCL internals and might be confusing the issue. From a user facing perspective, Would allowing those two forms of expression suffice for your use cases? I also think you could get fancy with it, without adding syntax sugar: resource "foo" "bar" {
enabled = var.isenabled
}
locals {
fallback = {attr = "default_value")
}
output "out" {
// Four Equivalent expressions
// 1. Use the same expression as the enabled field
value = var.isenabled ? foo.bar.attr : "default_value"
// 2. Check if the resource is null directly
value = foo.bar != null ? foo.bar.attr : "default_value"
// 3. Use coalesce with an inline default
value = coalesce(foo.bar, {attr = "default_value"}).attr
// 4. Use coalesce with a local default
value = coalesce(foo.bar, local.fallback).attr
} All four of those would work under the same set of logic I'm proposing above. I'd probably only ever use 2 or 4 myself, but it is flexible to do any of those four variants. |
All of these work beautifully for me 💯 |
Retranscripting my chat from github: The typical example is this:
In replacement of the common occurence i see which makes a [0] a part of weird code
it can then flow naturally considering the same syntax as depends_on, returning a bool in those circumstances it's being tested against:
I think it's a choice between introduing null or making the resource booley in those scenarios (null not being my favourite keyword in any language) |
Would this also be considered for dynamic blocks? resource "azurerm_linux_function_app" "function_app" {
// ...
site_config {
// ...
dynamic "application_stack" {
for_each = var.dotnet_version != "" ? [var.dotnet_version] : []
content {
dotnet_version = application_stack.value
use_dotnet_isolated_runtime = var.dotnet_isolated
}
}
// ...
}
// ...
} becomes something like resource "azurerm_linux_function_app" "function_app" {
// ...
site_config {
// ...
dynamic "application_stack" {
if = var.dotnet_version != ""
content {
dotnet_version = var.dotnet_version
use_dotnet_isolated_runtime = var.dotnet_isolated
}
}
// ...
}
// ...
} |
@tom-reinders I think it's a different set of technical issues, but should follow the same syntax. |
@tom-reinders @cam72cam - up to you but I don't see a need to change the dynamic blocks, at least not as a strict need. You can already create zero entries using the |
Don't want to distract from the main discussion but wanted to throw this out there; as a TF user that's been tracking hashicorp/terraform#21953 for some time (and the previous issue dating back to 2021), it appears there's a lot of things to think about from a backwards-compatibility perspective. FWIW, I'd happily refactor my code to a new major version if that's what it takes to get this over the line, instead of trying to design it to be perfectly backwards-compatible with all |
Speaking as a member of the community: @ikegentz But I just noticed this issue. Is However, probably in the future something like |
Sorry, my fault. |
We have discussed this with the core team and we feel that an RFC detailing the exact implementation would be good to get this issue moving. @cam72cam will write an RFC to detail the implementation. |
OpenTofu Version
Use Cases
We often use
count = var.xxxx ? 1 : 0
in various flavours but with the same purpose of determining to create or not create a single resource.While this is currently supported it means that resources require an indexed reference, and this also means that if you later change something to be conditional then you have a lot of manual state manipulation required.
I'd like to see this natively supported as it would simplify a lot of code.
Attempted Solutions
Workarounds are using
count = ...
expressions.Proposal
Add an additional meta-data atttribute
opentofu-options
and a sub-attributeresource-enabled
, for example:When a resource is not enabled (known at plan time), then any reference to it in the graph becomes invalid and this would prevent a bunch of munging such as
coalesce(aws-key_pair.my-key-pair.*.name, "")
or equivalent.References
enabled
parameter to avoid logical statements incount
hashicorp/terraform#21953 - long thread, lots of motivation, no movementThe text was updated successfully, but these errors were encountered: