Skip to content
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

Merged
merged 10 commits into from
Jun 4, 2024
Merged

stacks: update RPC APIs with deferral information #35125

merged 10 commits into from
Jun 4, 2024

Conversation

DanielMSchmidt
Copy link
Collaborator

@DanielMSchmidt DanielMSchmidt commented May 7, 2024

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.

@DanielMSchmidt DanielMSchmidt changed the title TF 13966 stacks: update RPC APIs with deferral information May 7, 2024
Copy link
Member

@alisdair alisdair left a 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:
Copy link
Member

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.

Copy link
Member

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.
Copy link
Member

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…

Copy link
Member

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.

@liamcervante liamcervante merged commit 59ead53 into main Jun 4, 2024
10 checks passed
@liamcervante liamcervante deleted the TF-13966 branch June 4, 2024 13:14
Copy link

github-actions bot commented Jun 4, 2024

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants