Simplifying Actions - prep PR

[mildwonkey]

This PR is half renaming things and half real changes.

• renamed many of the action-related nodes and some fields
  • LifecycleActionTrigger -> ResourceActionTrigger
  • action-related node names are closer to the resource node naming conventions (the simplifying actions refactor will make the naming clearer, but landing this now will make that PR much easier to read)
• added the ConfigAction to the ActionRef struct so we don't have to parse it multiple times
  • this change is why I removed the splat is disallowed test - we now catch that during configload (and with a different error)
• added some actions-related functions to support switching from actions.Actions to ctx.InstancesExpander (a future PR will add an AttachActionConfigTransformer to attach the action configuration to any node that needs it)

Actual changy-change:

• I refactored the invoke-specific action codepaths to use the instances expander (and available action config) instead of actions.Actions
  • I attempted the same in the generic actions codepaths, but that exposed an underlying race condition (missing dependency edges meant that the config action wasn't guaranteed to have been fully expanded when needed).

Since race conditions are an issue with actions, I offer this additional proof:

go test ./internal/terraform -count=50 -failfast         
ok  	github.com/hashicorp/terraform/internal/terraform	198.305s

Fixes #

Target Release

1.15.x

Rollback Plan

• If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

• This change is user-facing and I added a changelog entry.
• This change is not user-facing.

[DanielMSchmidt]

Since race conditions are an issue with actions, I offer this additional proof:

Huh, on main this command also works just fine and I never encountered a race condition in actions after the initial issues we had before the release 🤔

[mildwonkey]

@DanielMSchmidt yes; main isn't using the instances expander to validate action expansion. I'm sorry I wasn't clear: the race condition appears when the actions.Actions() struct is removed and we validate against the instance expander instead.

There is an underlying bug with the action graph, related to missing dependency edges during apply: #37975 . I believe that issue is directly related to the race condition in my POC.

[DanielMSchmidt]

renamed many of the action-related nodes and some fields: LifecycleActionTrigger -> ResourceActionTrigger

One thing I'd like to note down on the topic is that we have this name in all the protobuf definitions as well. If we rename it, it will be harder to find these places through a simple search and might be confusing. This includes all cross-repo naming as well (tfc-agent, atlas), so I just want to make sure we are clear on the trade-off of renaming this.

[mildwonkey]

renamed many of the action-related nodes and some fields: LifecycleActionTrigger -> ResourceActionTrigger

One thing I'd like to note down on the topic is that we have this name in all the protobuf definitions as well. If we rename it, it will be harder to find these places through a simple search and might be confusing. This includes all cross-repo naming as well (tfc-agent, atlas), so I just want to make sure we are clear on the trade-off of renaming this.

Good point! Since it's only in the planproto, and not the provider protocol, we can just change those names too - I'll add that change.

[DanielMSchmidt]

@mildwonkey We should inform upstream teams (provider, actions, etc) of this so they can also adjust their code properly 👍

[mildwonkey]

@DanielMSchmidt the provider protocol is not changing; no providers need to change anything

[DanielMSchmidt]

@mildwonkey I am aware, but if providers or the cloud codebase in their code talk about LifecycleActionTriggers it would be beneficial if they would rename that as well so that we all talk about the same thing.

[mildwonkey]

That's a good point, I'll definitely spread the info around - we did start removing references to lifecycle actions back before the actual release, so I'm hoping it's not around too many other places: #37533

[DanielMSchmidt]

👍 Yeah, I think we still distinguish between CLI triggered and Lifecycle triggered actions and LifecycleActionTrigger is the currently prevalent name for the latter :)

[mildwonkey]

lol so something went very wrong when i squashed my last commit (actually post squash, when I tried to reset something) and now most of my changes are gone. I'll open this again and do a better job with the commits while i'm at it 🤦🏻

sorry for all the noise!