From 0f936b9d80c29d653811e8a15265911bdbd9c902 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 7 May 2021 10:18:59 -0700 Subject: [PATCH] plans/planfile: Read state snapshots as part of reading a plan 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. --- plans/planfile/planfile_test.go | 9 +++++++++ plans/planfile/reader.go | 30 +++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/plans/planfile/planfile_test.go b/plans/planfile/planfile_test.go index a25a9f79f..765f0892a 100644 --- a/plans/planfile/planfile_test.go +++ b/plans/planfile/planfile_test.go @@ -60,6 +60,15 @@ func TestRoundtrip(t *testing.T) { Config: plans.DynamicValue([]byte("config placeholder")), Workspace: "default", }, + + // Due to some historical oddities in how we've changed modelling over + // time, we also include the states (without the corresponding file + // headers) in the plans.Plan object. This is currently ignored by + // Create but will be returned by ReadPlan and so we need to include + // it here so that we'll get a match when we compare input and output + // below. + PrevRunState: prevStateFileIn.State, + PriorState: stateFileIn.State, } workDir, err := ioutil.TempDir("", "tf-planfile") diff --git a/plans/planfile/reader.go b/plans/planfile/reader.go index b2252aa89..83b17ae9c 100644 --- a/plans/planfile/reader.go +++ b/plans/planfile/reader.go @@ -88,7 +88,35 @@ func (r *Reader) ReadPlan() (*plans.Plan, error) { } defer pr.Close() - return readTfplan(pr) + // There's a slight mismatch in how plans.Plan is modeled vs. how + // the underlying plan file format works, because the "tfplan" embedded + // file contains only some top-level metadata and the planned changes, + // and not the previous run or prior states. Therefore we need to + // build this up in multiple steps. + // This is some technical debt because historically we considered the + // planned changes and prior state as totally separate, but later realized + // that it made sense for a plans.Plan to include the prior state directly + // so we can see what state the plan applies to. Hopefully later we'll + // clean this up some more so that we don't have two different ways to + // access the prior state (this and the ReadStateFile method). + ret, err := readTfplan(pr) + if err != nil { + return nil, err + } + + prevRunStateFile, err := r.ReadPrevStateFile() + if err != nil { + return nil, fmt.Errorf("failed to read previous run state from plan file: %s", err) + } + priorStateFile, err := r.ReadStateFile() + if err != nil { + return nil, fmt.Errorf("failed to read prior state from plan file: %s", err) + } + + ret.PrevRunState = prevRunStateFile.State + ret.PriorState = priorStateFile.State + + return ret, nil } // ReadStateFile reads the state file embedded in the plan file, which