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

allow filtering elements with plain non-transforming for_each #35347

Open
ryenus opened this issue Jun 16, 2024 · 6 comments
Open

allow filtering elements with plain non-transforming for_each #35347

ryenus opened this issue Jun 16, 2024 · 6 comments
Labels
enhancement hcl Use in conjunction with "upstream" when HCL is the relevant upstream new new issue not yet triaged upstream

Comments

@ryenus
Copy link

ryenus commented Jun 16, 2024

Terraform Version

Terraform v1.8.5
on darwin_arm64

Use Cases

filtering a map while iterating with for_each

Attempted Solutions

resource "the_type" "the_name" {
  for_each = { 
    for key, value in var.map_of_objects : key => value if value.enabled == true
  }

  a = each.value.a
}

Proposal

resource "the_type" "the_name" {
  for_each = var.map_of_objects if each.value.enabled == true

  a = each.value.a
}

Since we're only filtering the elements in the input variable "map_of_objects", without performing any transformation. It would be great if we can omit the for expression and hoist the filtering expression

References

No response

@ryenus ryenus added enhancement new new issue not yet triaged labels Jun 16, 2024
@ryenus ryenus changed the title allowing filtering elements with plain for_each allowing filtering elements with plain non-transorming for_each Jun 16, 2024
@ryenus ryenus changed the title allowing filtering elements with plain non-transorming for_each allow filtering elements with plain non-transorming for_each Jun 16, 2024
@ryenus ryenus changed the title allow filtering elements with plain non-transorming for_each allow filtering elements with plain non-transforming for_each Jun 16, 2024
@apparentlymart
Copy link
Member

apparentlymart commented Jun 17, 2024

Thanks for this idea, @ryenus.

I think one important implication of your proposed shorthand is that it replaces with explicit declaration of local symbols key, value with something implicit, which in your case you've decided is an each object with key and value attributes similar to what the for_each feature itself implicitly declares for the other expressions in the block.

That isn't necessarily disqualifying, but it does violate the current separation of concerns in how Terraform is built: for_each and the each global symbol belong to Terraform, while for expressions (and their explicit symbol declarations) belong to the underlying toolkit that the Terraform language is built from, HCL. HCL itself does not currently have any features that cause symbols to be implicitly placed into the evaluation scope without being assigned an explicit name by the author.

A variation of this that would not raise these concerns would be to retain the overall for grammar but allow the : result clause to be omitted:

for_each = { 
  for key, value in var.map_of_objects
  if value.enabled == true
}

This retains the explicit declaration of key and value, and retains the {/} delimiters which signal that the result will be an object. My assumption here is that if the : result clause is omitted then the resulting element is always exactly the same as the input element, which implies that the collection for the in clause must be of a mapping type when using {/} or must be a sequence type when using [/].


(I remember that the colon ended up being part of the for grammar to resolve a parsing ambiguity in finding the end of the in clause, so in considering either of these proposals we'll need to remind ourselves about what that ambiguity was and determine whether the if keyword is a sufficient substitute to mark the end of the collection expression.)

@crw
Copy link
Collaborator

crw commented Jun 17, 2024

Thanks for this feature request! If you are viewing this issue and would like to indicate your interest, please use the 👍 reaction on the issue description to upvote this issue. We also welcome additional use case descriptions. Thanks again!

@apparentlymart apparentlymart added upstream hcl Use in conjunction with "upstream" when HCL is the relevant upstream labels Jun 17, 2024
@ryenus
Copy link
Author

ryenus commented Jun 20, 2024

@apparentlymart & @crw, thanks a lot for looking into this.

I don't have much insight into the HCL internals yet, but regarding the visibility and/or scope of each, I thought it could be the same each when used to assigning the arguments? I updated the code snippet in the top comment for clarification.

@apparentlymart
Copy link
Member

Hi @ryenus,

The main tricky thing here is that for expressions can appear anywhere that general expressions are allowed, including inside other for expressions or other language features that declare their own local symbols, and so if the name each were hard-coded then the author would have no recourse if the symbol each were also already in use for something else. That's particularly problematic for the name each in particular because Terraform has already reserved that name for use with resource and module for_each, and already doesn't let the author select a different name.

Using a fixed name in the design of resource and module for_each does not cause this problem because these objects cannot nest inside one another and so at any point in the configuration there is always either zero or one features defining the each symbol.

For anything that's more general we need to provide a way for the local symbol to vary based on context so that the author can select a name that isn't already being used for something else. That's why, for example, dynamic blocks use the name of the block type as the iterator symbol by default, and allow the author to override that when even that isn't unique enough (such as when a blocks of the same type name nest inside one another).

My proposed modification was therefore intending to still provide the necessary way to choose the local symbol names, while removing the redundant transformation clause is the case where only filtering is desired and no transformation is needed.

@ryenus
Copy link
Author

ryenus commented Jun 21, 2024

Is it possible to treat the proposed one as a syntactic sugar, then leverage something like a preprocessor to implicitly convert it to the one with an explicit for expression? So that maybe the two can have the same semantics?

@jbardin
Copy link
Member

jbardin commented Jun 21, 2024

Adding a second pair of eyes here, the original proposal is difficult to read. You are still transforming the value -- you intended to write an expression which takes a map, transform it by filtering keys into a new possibly smaller map, and assign the resulting value to for_each.

If I were to see the expression for_each = var.map_of_objects if each.value.enabled == true for the first time, it reads like an incomplete conditional expression, where you would expect the result to be var.map_of_objects, OR something else which is implied by the missing else clause, maybe nil?

Aside from the fact that it may not be possible to parse correctly (haven't given that any thought, but the scope issues were mentioned above), wrapping the expression in {...} makes it clear what is being processed, and keeps things consistent with the existing language grammar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement hcl Use in conjunction with "upstream" when HCL is the relevant upstream new new issue not yet triaged upstream
Projects
None yet
Development

No branches or pull requests

4 participants