remove the use of data source prior state from planning #30832
Conversation
After data source handling was moved from a separate refresh phase into the planning phase, reading the existing state was only used for informational purposes. This had been reduced to reporting warnings when the provider returned an unexpected value to try and help locate legacy provider bugs, but any actual issues located from those warnings were very few and far between. Because the prior state cannot be reliably decoded when faced with incompatible provider schema upgrades, and there is no longer any significant reason to try and get the prior state at all, we can skip the process entirely.
Data sources should not require reading the previous versions. While we previously skipped the decoding if it were to fail, this removes the need for any prior state at all. The only place where the prior state was functionally used was in the destroy path. Because a data source destroy is only for cleanup purposes to clean out the state using the same code paths as a managed resource, we can substitute the prior state in the change change with a null value to maintain the same behavior.
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/hashicorp/terraform/E2RYCnftpwH4WghxQVLhJjfSMt7P |
jbardin
changed the title
| @@ -1515,9 +1512,6 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, currentSt | |||
| ) | |||
| diags = diags.Append(checkDiags) | |||
| if diags.HasErrors() { | |||
| diags = diags.Append(ctx.Hook(func(h Hook) (HookAction, error) { | |||
| return h.PostApply(n.Addr, states.CurrentGen, priorVal, diags.Err()) | |||
| })) | |||
There was a problem hiding this comment.
I was a bit worried here that we'd break the timer-based "Still reading…" UI hooks here by sometimes calling PreApply without a corresponding PostApply, but after trying to trace through the code to see if that's possible I'm completely lost 😵💫 Just raising it in case you hadn't considered it and think it's worth looking into.
There was a problem hiding this comment.
Thanks! I could not find where this was used at all, and wasn't surprised since most of the hooks are no longer used. I didn't think about the "Still reading..." output though, so I'll walk back from there once to doublecheck before merging.
There was a problem hiding this comment.
OK, this is not even the correct place to call PostApply, since this section has nothing to do with reading the data source. The PostApply hook is still called after ReadDataSource, which corresponds to the ui output.
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
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. |
After data source handling was moved from a separate refresh phase into the planning phase, reading the existing state was only used for informational purposes. This had been reduced to reporting warnings when the provider returned an unexpected value to try and help locate legacy provider bugs, but any actual issues located from those warnings were very few and far between.
Because the prior state cannot be reliably decoded when faced with incompatible provider schema upgrades, and there is no longer any significant reason to try and get the prior state at all, we can skip the process entirely.
The only place where the prior state was functionally used was in the destroy path to create the change entry. Because a data source destroy is only for cleanup purposes to clean out the state using the same code paths as a managed resource, we can substitute the prior state in the change with a null value to maintain the same behavior.
This is a more complete followup to #30830, not intended for backport.