Skip to content

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

Merged
liamcervante merged 1 commit intomainfrom
liamcervante/35630
Sep 11, 2024
Merged

plan renderer: fix crash when updating a null attribute to unknown#35709
liamcervante merged 1 commit intomainfrom
liamcervante/35630

Conversation

@liamcervante
Copy link
Copy Markdown
Contributor

@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

@liamcervante liamcervante requested a review from a team September 11, 2024 09:49
@kmoe
Copy link
Copy Markdown
Member

kmoe commented Sep 11, 2024

Is this the right base branch?

@liamcervante
Copy link
Copy Markdown
Contributor 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
Copy Markdown
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
@liamcervante liamcervante deleted the liamcervante/35630 branch September 11, 2024 12:27
@github-actions
Copy link
Copy Markdown
Contributor

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

@github-actions
Copy link
Copy Markdown
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

2 participants