Skip to content

actions: remove actions.Action global registry and replace with existing patterns#38208

Open
mildwonkey wants to merge 2 commits intomainfrom
mildwonkey/action-expansion
Open

actions: remove actions.Action global registry and replace with existing patterns#38208
mildwonkey wants to merge 2 commits intomainfrom
mildwonkey/action-expansion

Conversation

@mildwonkey
Copy link
Contributor

@mildwonkey mildwonkey commented Feb 24, 2026

Actions were using a global registry of (evaluated) action configurations to pass action config between Action nodes and other nodes which needed the config (such as ActionTrigger Nodes, which handle the actual action apply step). This commit replaces that pattern with the instances expander + explicitly passing action configs between nodes types during transformation and expansion, bringing actions closer in line with other resources.

It's theoretically possible that users will notice this change: we were previously storing evaluated action configuration, and now that configuration is re-evaluated every time (right before plan and apply) so it's possible an action could be planned or applied with different values (values which were updated during the graph walk) in this implementation.

The general strategy for removing the action global registry (actions.Actions) was as follows:

  • replaced GetActionInstanceKeys(addr) with ctx.InstanceExpander().ExpandAction(addr) (drop in replacement)
  • added support for HasAction/HasActionInstance in instances.Set (returned by the expander above) for individual action (or action instance) validation (removing the need for GetAction/GetActionInstance for validation)
  • adding action config (configs.Action) to nodeActionTriggerApplyInstance so the raw config can get re-evaluated before plan and apply, removing the need for GetAction/GetActionInstance for config
  • added ActionByAddr to configs.Module to enable easy access to raw action configuration (used for the above, in ActionInvokeApplyTransformer)
  • removed actions.Actions() from Context

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.

@mildwonkey mildwonkey force-pushed the mildwonkey/action-expansion branch 3 times, most recently from bb6b464 to f1749d5 Compare February 24, 2026 17:39
@mildwonkey mildwonkey marked this pull request as ready for review February 24, 2026 17:55
@mildwonkey mildwonkey requested a review from a team as a code owner February 24, 2026 17:55
@mildwonkey mildwonkey requested review from jbardin and kmoe February 24, 2026 18:54
}
}

ctx.Actions().AddActionInstance(n.Addr, configVal, n.ResolvedProvider)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this node's only purpose was to record the config in Actions(). These diags should probably come from somewhere else now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was a good prompt to copy the deprecations bits around.

This caused a slight change in the deprecation tests, specific to deprecations that are caught at plan/apply: the action has to be actually triggered for the deprecation to be found, since we only evaluate* the configuration in the node_action_trigger_instance_plan/apply (*re-evaluate after Validate, at least)

The other thing this Execute does is check for the action deferral. I'm not going to change anything else there right now, that's not surviving the refactor anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, deprecations are somewhat dynamic, and can't always be caught without the actual data being used. While instances are only evaluated if they are triggered, there might be a place to evaluate the config in a validate context somewhere, either within resource validation or maybe a specific node (but I'd rather not have the validation graph structured differently if we can help it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding this to the list for the refactor - I can look into attaching action configs to resource nodes during validate just for validation purposes.

Unless this was something you wanted me to fix in this PR? I can also revert this change and leave this node's Execute as-is, either is fine with me!

@mildwonkey mildwonkey added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Feb 24, 2026
@mildwonkey mildwonkey force-pushed the mildwonkey/action-expansion branch from 992fa8c to 25af9b4 Compare February 24, 2026 22:01
@mildwonkey mildwonkey requested a review from jbardin February 24, 2026 22:07
@mildwonkey mildwonkey force-pushed the mildwonkey/action-expansion branch from 25af9b4 to 410d439 Compare February 26, 2026 15:10
jbardin
jbardin previously approved these changes Feb 26, 2026
…ing patterns

Actions were using a global registry of (evaluated) action configurations to pass action config between Action nodes and other nodes which needed the config (such as ActionTrigger Nodes, which handle the actual action apply). This commit replaces that pattern with the instances expander + explicity passing action configs between nodes types during transformation and expansion, bringing actions closer in line with other resources.
@mildwonkey
Copy link
Contributor Author

@jbardin sorry for the ping but I noticed a missing deprecation check in node_action_invoke and need a re-review (I plan on extracting the config processing into a method i can re-use in the next PR so this doesn't continue to be a thing)

@kmoe
Copy link
Member

kmoe commented Mar 4, 2026

I think this is user-facing because in theory this could change behaviour in some circumstances.

@mildwonkey mildwonkey requested a review from jbardin March 4, 2026 19:28
@mildwonkey
Copy link
Contributor Author

I dismissed @jbardin 's approval after discussion with him and @kmoe - we're going to wait for the remaining refactor on this change , as it's not clear if there will be user-visible impact or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog-needed Add this to your PR if the change does not require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants