Evaluate resource preconditions and postconditions during apply even if they have no planned change#31491
Merged
apparentlymart merged 2 commits intomainfrom Jul 22, 2022
Merged
Conversation
We previously would optimize away the graph nodes for any resource instance without a real change pending, but that means we don't get an opportunity to re-check any invariants associated with the instance, such as preconditions and postconditions. Other upstream changes during apply can potentially decide the outcome of a condition even if the instance itself isn't being changed, so we do still need to revisit these during apply or else we might skip running certain checks altogether, if they yielded unknown results during planning and then don't get run during apply.
apparentlymart
force-pushed
the
f-apply-noop-conditions
branch
from
e9b6889 to
d1ec778
Compare
Previously we tried to early-exit before doing anything at all for any no-op changes, but that means we also skip some ancillary steps like evaluating any preconditions/postconditions. Now we'll skip only the main action itself for plans.NoOp, and still run through all of the other side-steps. Since one of those other steps is emitting events through the hooks interface, this means that now no-op actions are visible to hooks, whereas before we always filtered them out before calling. I therefore added some additional logic to the hooks to filter them out at the UI layer instead; the decision for whether or not to report that we visited a particular object and found no action required seems defensible as a UI-level concern anyway.
apparentlymart
force-pushed
the
f-apply-noop-conditions
branch
from
d1ec778 to
91cb440
Compare
jbardin
approved these changes
Jul 22, 2022
Contributor
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preconditions and postconditions results can change due to changes to other upstream resources, even if the resource they are directly attached to doesn't have a change pending.
Therefore we need to still visit "no-op" resources during the apply walk and deal with the ancillary side-effects, skipping only the actual call to the provider to run
ApplyResourceChange. If we don't, there can potentially be failures we won't catch until the next plan, which might be run by someone who has no context about whatever change broke the conditions.This also makes the UI "see" no-op changes being applied via the hooks API. Rather than making another special case in Terraform Core for that, I concluded it's a UI-level concern to decide whether it's interesting to report a particular resource being "skipped" and so I put the special case in the UI layer instead.
Fixes #31261.