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

plan renderer: fix crash when updating a null attribute to unknown #35709

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

liamcervante
Copy link
Member

@liamcervante liamcervante commented Sep 11, 2024

This PR fixes a crash in the plan renderer that occurs when a plan marks an attribute that was null as becoming unknown.

The renderer had assumed that all computed attributes would have a value if the resource was being updated, so was only checking for plans.Create actions before deciding whether to render the "before" part of an unknown change. Now, we explicitly check whether the before part has a renderer at all when deciding whether to render the before part rather than simply relying on a logical consistency based on the false assumption it would only ever be create actions doing this.

There's also a flyby fix of the block renderers which can now successfully render blocks becoming unknown as update actions instead of create since that does not crash anymore.

Note, we could render this as something like ~ attr = null -> (known after apply), instead of just ~ attr = (known after apply), but I don't think we do that elsewhere (like when an optional attribute is set during an update operation) so I didn't implement it here but it would be trivial to do.

Fixes #35630

Target Release

v1.9.7

Draft CHANGELOG entry

BUG FIXES

  • Plan renderer: Fix crash that occurs when updating a null attribute to unknown

@kmoe
Copy link
Member

kmoe commented Sep 11, 2024

Is this the right base branch?

@liamcervante
Copy link
Member Author

Is this the right base branch?

Yeah, this PR has a dependency on #35644 as I wanted to make the other fix work in a certain way that this crash was preventing. But the root cause of the two issues are different, and I thought it was nicer to fix them in dedicated PRs for accounting purposes.

@kmoe
Copy link
Member

kmoe commented Sep 11, 2024

Aha, I didn't see that one, thanks!

@liamcervante liamcervante added the 1.9-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Sep 11, 2024
Base automatically changed from liamcervante/35556 to main September 11, 2024 12:20
@liamcervante liamcervante merged commit 3327578 into main Sep 11, 2024
5 of 6 checks passed
@liamcervante liamcervante deleted the liamcervante/35630 branch September 11, 2024 12:27
Copy link
Contributor

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

Copy link
Contributor

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 Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.9-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform Crash
3 participants