-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
Conversation
AFTER_UPDATE = 4; | ||
BEFORE_DESTROY = 5; | ||
AFTER_DESTROY = 6; | ||
CLI = 7; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 :)
There was a problem hiding this 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.
…e providers they need
this validates that the provider must stay open long enough for the resource to plan the actions
Co-authored-by: Liam Cervante <liam.cervante@hashicorp.com>
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
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry