Skip to content

actions: connect resource instance nodes to after actions #37936

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.

Sign up for GitHub

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

Jump to bottom
Merged
DanielMSchmidt merged 3 commits into from
Nov 24, 2025
Merged

actions: connect resource instance nodes to after actions #37936

DanielMSchmidt merged 3 commits into from
Nov 24, 2025
+148 −8

Conversation

@DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented Nov 24, 2025 •
edited
Loading

Fixes #37930

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 DanielMSchmidt requested a review from a team as a code owner November 24, 2025 13:59
@DanielMSchmidt DanielMSchmidt force-pushed the fix-expanded-resource-after-action-ordering branch from 9b7bfe4 to f0abdc3 Compare November 24, 2025 14:00
@DanielMSchmidt DanielMSchmidt added the 1.14-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Nov 24, 2025
@DanielMSchmidt DanielMSchmidt force-pushed the fix-expanded-resource-after-action-ordering branch from f0abdc3 to 1784fb8 Compare November 24, 2025 14:01
jbardin
jbardin reviewed Nov 24, 2025
View reviewed changes
internal/terraform/transform_action_trigger_config.go Outdated
CreateNodesAsAfter bool
ConcreteActionTriggerNodeFunc ConcreteActionTriggerNodeFunc
CreateNodesAsAfter bool
ConnectToResourceInstanceNodes bool // if false it connects to resource nodes instead of resource instance nodes
Copy link
Member

@jbardin jbardin Nov 24, 2025

Choose a reason for hiding this comment

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

Normally if something connects to a resource, it can connect to both the expand node and the instance nodes, and there is no need to differentiate between the two.

Copy link
Contributor

@mildwonkey mildwonkey Nov 24, 2025

Choose a reason for hiding this comment

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

This isn't a direct request, you don't need to make this change now, but I was curious about this section when I first saw it: Given that these variables only change from default if we're in apply mode(in the graph builder code); are they even necessary in this struct? Can we set these as helper variables directly in the Transformer based on the operation (t.operation)?

Copy link
Contributor Author

@DanielMSchmidt DanielMSchmidt Nov 24, 2025

Choose a reason for hiding this comment

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

Okay, yeah I can connect both, no problem

@mildwonkey
Copy link
Contributor

mildwonkey commented Nov 24, 2025

output

The graph edges look correct now! I think James had good feedback; we can connect to both the resource and resource instances, but otherwise this looks good!

@DanielMSchmidt
Copy link
Contributor Author

DanielMSchmidt commented Nov 24, 2025

Screenshot 2025-11-24 at 18 31 38

Verified that it fixes the issue at hand

@mildwonkey
Copy link
Contributor

mildwonkey commented Nov 24, 2025

nice, I see the new edges in the graph (this is the graph just before the pruning transformers run; the "redundant" edges don't show up in the final graph):

2025-11-24T12:46:12.953-0500 [TRACE] (graphTransformerMulti) Completed graph transform *terraform.ProviderTransformer with new graph:
  action.local_command.bar (expand) - *terraform.nodeExpandActionDeclaration
    provider["registry.terraform.io/hashicorp/local"] - *terraform.NodeApplyableProvider
  action.local_command.bar triggered by local_file.foo (apply - after) - *terraform.nodeActionTriggerApplyExpand
    local_file.foo - *terraform.NodeApplyableResourceInstance
    local_file.foo (expand) - *terraform.nodeExpandApplyableResource
    provider["registry.terraform.io/hashicorp/local"] - *terraform.NodeApplyableProvider
  local_file.foo - *terraform.NodeApplyableResourceInstance
    local_file.foo (expand) - *terraform.nodeExpandApplyableResource
    provider["registry.terraform.io/hashicorp/local"] - *terraform.NodeApplyableProvider
  local_file.foo (expand) - *terraform.nodeExpandApplyableResource
    provider["registry.terraform.io/hashicorp/local"] - *terraform.NodeApplyableProvider
  provider["registry.terraform.io/hashicorp/local"] - *terraform.NodeApplyableProvider

A bunch of transformers run (rather quietly) after this graph, and we end up with the same graph I posted from the first version 👍🏻

mildwonkey
mildwonkey approved these changes Nov 24, 2025
View reviewed changes
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

lgtm, and extra thanks for reducing the scope on those variables!

@DanielMSchmidt DanielMSchmidt merged commit 65a23ed into main Nov 24, 2025
13 checks passed
@DanielMSchmidt DanielMSchmidt deleted the fix-expanded-resource-after-action-ordering branch November 24, 2025 19:49
@github-actions github-actions bot mentioned this pull request Nov 24, 2025
Merged
3 tasks
@fox-md fox-md mentioned this pull request Dec 5, 2025
Open
@github-actions
Copy link
Contributor

github-actions bot commented Dec 25, 2025

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mildwonkey mildwonkey mildwonkey approved these changes

@jbardin jbardin Awaiting requested review from jbardin

Assignees

No one assigned

Labels

1.14-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

lifecycle.action_trigger executes actions before resource creation/update is completed

3 participants

@DanielMSchmidt @mildwonkey @jbardin