-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
stacks: update RPC APIs with deferral information #35125
Conversation
moving the components out of the main function definition so that we can reuse the implementation for deferred resource instances which wraps the message used for PlannedChangeResourceInstancePlanned
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 looks good to me, as far as I understand it!
Are we intentionally not adding deferral data to the JSON change description documents in this PR, or do I misunderstand something?
@@ -204,6 +206,79 @@ func LoadFromProto(msgs []*anypb.Any) (*Plan, error) { | |||
c.ResourceInstancePriorState.Put(fullAddr, nil) | |||
} | |||
|
|||
case *tfstackdata1.PlanDeferredResourceInstanceChange: |
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.
There's a lot of overlap between this case and the planned change, which makes sense. Do you think there might be some way to unify them, maybe with helper functions? I'm a bit concerned about logic drift over time if the code is duplicated.
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.
Done!
@@ -277,18 +277,12 @@ func (pc *PlannedChangeResourceInstancePlanned) PlannedChangeProto() (*terraform | |||
// if this plan later gets applied. | |||
// We only emit a "raw" in this case, because this is a relatively | |||
// uninteresting edge-case. |
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 not sure this comment fits here any more, although I am having some trouble with GitHub's diff of this file…
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.
Moved the comments around so they line up properly again.
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
This PR adds representation of deferred changes into the stacks RPC API. We create a new kind of planned change (a deferred resource) and emit that whenever we discover deferred changes from Terraform Core.