fix: move parsing action reference expressions into terraform

[mildwonkey]

Previously, I was erroneously storing action and linkedResource expressions as Traversals during config loading. This PR replaces those fields with hcl.Expression, moves some validation around to account for the fact we don't have fully evaluated traversals yet (some checks have been removed entirely and will be replaced in a follow up PR, I wanted to get this right first), and finally moves the

I stumbled upon the jsonconfig package at some point in all this, so that first file is really just opportunistic.

To solve this, I added a permissive parsing of the ActionExpression during ActionPlanTransformer.transformSingle (to get the config action) and store the action expression inside the nodeActionTriggerPlanExpand.lifecycleActionTrigger so it can be fully evaluated (with access to the resource's repetition data, which is necessary if the action in the resource's lifecycle block uses count.index or for_each.key) during nodeActionTriggerPlanExpand.DynamicExpand

[DanielMSchmidt]

Looks good to me, does this mean we can now enable this test?

[mildwonkey]

Looks good to me, does this mean we can now enable this test?

Nope, but thanks for the reminder 😅 - enabling it confirmed my concerns re: additional refs and how I'm handling count/foreach in the expression

    --- FAIL: TestContextPlan_actions/expanded_resource_-_expanded_action (0.00s)
panic: unexpected action address addrs.CountAttr [recovered, repanicked]

[matejrisek]

Out of curiosity and for my learning - why remove quotation marks?

There are more such instances in this very file and I a quick search shows that we use them for this purpose in other parts of the code.

[mildwonkey]

I was seeing those in tests where none of the other messages had quotes around the attribute, so I removed them, but you make a good point that actually did nothing for consistency.

[mildwonkey]

I'll add diagnostics to the (not-yet-extant) list of things that need style guides 😁

[DanielMSchmidt]

Looks good to me 👍 I'd like to have more test cases around scenarios we don't support (like using locals.my_action_list, concat, etc), but we can also add them in a follow up :)

[DanielMSchmidt]

Wouldn't we want to validate that this is a reference to an action or action instance here or is that done somewhere else?

[mildwonkey]

We do, during config load: https://github.com/hashicorp/terraform/blob/main/internal/configs/action.go#L157-L163

[DanielMSchmidt]

Can't we use expander.GetResourceInstanceRepetitionData(addr addrs.AbsResourceInstance) here? The resource should have run before, because actions always plan after their triggering resource.

[mildwonkey]

Maybe! Most of this was copied from the replace_trigger_by evaluation code and I didn't change anything I didn't have an explicit need to change.

[DanielMSchmidt]

👍 on the rename, it bothered me a bit but I always forgot again before taking action (pun intended)

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.