Skip to content

Add actions to the apply graph #37349

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

Merged
merged 22 commits into from
Jul 24, 2025
Merged

Add actions to the apply graph #37349

merged 22 commits into from
Jul 24, 2025

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented Jul 18, 2025

This PR adds actions to the apply graph. There are a bunch of features within actions missing (conditions / provider attribute) and a lot of tests missing, but this should build a good basis to expand from.

Fixes TF-25937

Target Release

1.14.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 added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Jul 18, 2025
@DanielMSchmidt DanielMSchmidt marked this pull request as ready for review July 18, 2025 19:35
@DanielMSchmidt DanielMSchmidt requested a review from a team as a code owner July 18, 2025 19:35
AFTER_UPDATE = 4;
BEFORE_DESTROY = 5;
AFTER_DESTROY = 6;
CLI = 7;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about this - Triggers and Events are configuration constructs that don't apply to CLI triggered actions, so I wouldn't expect them to be included at all with a CLI triggered action. How is this info getting used? (I'm worried it'll be confusing if we present info about events and triggers for a CLI-invoked action which has neither of these)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This info is being used a) for constructing the apply graph and b) to display when we render the plan.
I'm also not 100% happy with the structure here. When building the prototype I experimented with a oneof solution differentiating between CLI and lifecycle triggered actions but that was creating a lot of boilerplate so I went with a flat structure here. I think in the renderer and apply graph we can check if the trigger event is CLI and if that's the case we don't read the other fields.

We can change the approach though at any time, so if we find this to be really annoying when building the terraform invoke command we can still adjust :)

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.

I left a couple of tiny comments but things look solid to me - I don't feel confident approving this, but that's not because I have major concerns.

@DanielMSchmidt DanielMSchmidt merged commit 92db9b8 into main Jul 24, 2025
15 of 16 checks passed
@DanielMSchmidt DanielMSchmidt deleted the TF-25937 branch July 24, 2025 09:28
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