plans/planfile: Read state snapshots as part of reading a plan #28653
Conversation
This file contains 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
Our model for plans/planfile has unfortunately grown inconsistent with changes to our modeling of plans.Plan. Originally we considered the plan "header" and the planned changes as an entirely separate artifact from the prior state, but we later realized that carrying the prior state around with the plan is important to ensuring we always have enough context to faithfully render a plan to the user, and so we added the prior state as a field of plans.Plan. More recently we've also added the "previous run state" to plans.Plan for similar reasons. Unfortunately as a result of that modeling drift our ReadPlan method was silently producing an incomplete plans.Plan object, causing use-cases like "terraform show" to produce slightly different results due to the plan object not round-tripping completely. As a short-term tactical fix, here we add state snapshot reading into the ReadPlan function. This is not an ideal solution because it means that in the case of applying a plan, where we really do need access to the state _file_, we'll end up reading the prior state file twice. However, the goal here is only to heal the modelling quirk with as little change as possible, because we're not currently at a point where we'd be willing to risk regressions from a larger refactoring.
apparentlymart
added
the
0.15-backport
alisdair
approved these changes
May 10, 2021
apparentlymart
removed
the
0.15-backport
|
Backported to v0.15 as 7a6c1a3 |
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Our model for
plans/planfilehas unfortunately grown inconsistent with changes to our modeling ofplans.Plan.Originally we considered the plan "header" and the planned changes as an entirely separate artifact from the prior state, but we later realized that carrying the prior state around with the plan is important to ensuring we always have enough context to faithfully render a plan to the user, and so we added the prior state as a field of
plans.Plan. More recently we've also added the "previous run state" toplans.Planfor similar reasons.Unfortunately as a result of that modeling drift our
ReadPlanmethod was silently producing an incompleteplans.Planobject, causing use-cases liketerraform showto produce slightly different results due to the plan object not round-tripping completely.As a short-term tactical fix, here we add state snapshot reading into the
ReadPlanfunction. This is not an ideal solution because it means that in the case of applying a plan, where we really do need access to the state file, we'll end up reading the prior state file twice. However, the goal here is only to heal the modelling quirk with as little change as possible, because we're not currently at a point where we'd be willing to risk regressions from a larger refactoring.I came across this while working on #28634. Without the changes from that PR the discrepancies aren't a big deal for
terraform showyet, but I wanted to submit this separately anyway because that PR is getting kinda bulky and this bug isn't directly related to it.