-
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
Finish implementation of unknown count/for_each in data sources #35122
Conversation
I checked this with James, and it was to temporarily speed up test cycle times on a slippery bug.
…Allowed` Some of the methods on `Deferred` were using `d.externalDependencyDeferred` to report whether some items were deferred, without first checking that `d.deferralAllowed` was even turned on. Then, to accommodate that, we were making sure to only pass the external dependency deferral signal (AKA the whole component is deferred) in to the `Deferred` struct if deferral is allowed. This was an artifact of the order things got implemented in; the "is thing deferred" checks predated the `deferralAllowed` field, and the call sites of those methods were behind other guards at the time. Anyway, this commit flips things so we now always propagate the received `ExternalDependencyDeferred` value in to the `Deferred`, but the `Deferred` takes the global on/off switch into account before relying on that value for its final answers about a given possible deferral.
This should be sound 100% of the time, because the only way anything can be deferred in an apply is if we _planned_ to defer it... in other words, if deferral was allowed during the plan. The reason we need this enabled is to allow proper construction of wildcard key addresses for data sources that are deferred due to unknown count or for_each values. Unlike with resources, we don't have a convenient space in the plan to stash info about a data source deferral; data sources don't have "planned changes" per se (they're unmanaged and thus can't be changed), so they don't result in a "deferred change" and must be re-checked during the apply. However, since applies are relying on the plan's info about deferred _actual_ changes, they don't need to construct the (still somewhat expensive) resource graph, so we should opt them out of that graph construction to avoid a performance regression.
I think you will need to look for a deferred data source by checking for the absence of the other options.
I think the short circuit I also think allowing |
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
(Changelog already has a line to this effect, so I'm leaving it be.) |
This PR adds tests for deferring data sources due to unknown count/for_each values, and makes the necessary behavior changes to get those tests passing.
For reviewers: here's the main things to validate about this PR.
deferralAllowed = true
in applies? If not, can we think of a safer way to re-process data sources that should be deferred (preferably without significantly changing the semantics of the plan)?Deferred
's informational methods to do early negative returns ifdeferralAllowed == false
?Target Release
1.9.x
Draft CHANGELOG entry
EXPERIMENTS
The deferred actions experiment now supports deferring data sources due to unknown count/for_each values.