From 7894c7a3556e88af23dd4b46bceccded0100c586 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 5 Nov 2019 17:20:26 -0800 Subject: [PATCH 1/2] command: "terraform show" renders plans like "terraform plan" During the Terraform 0.12 work we briefly had a partial update of the old Terraform 0.11 (and prior) diff renderer that could work with the new plan structure, but could produce only partial results. We switched to the new plan implementation prior to release, but the "terraform show" command was left calling into the old partial implementation, and thus produced incomplete results when rendering a saved plan. Here we instead use the plan rendering logic from the "terraform plan" command, making the output of both identical. Unfortunately, due to the current backend architecture that logic lives inside the local backend package, and it contains some business logic around state and schema wrangling that would make it inappropriate to move wholesale into the command/format package. To allow for a low-risk fix to the "terraform show" output, here we avoid some more severe refactoring by just exporting the rendering functionality in a way that allows the "terraform show" command to call into it. In future we'd like to move all of the code that actually writes to the output into the "command" package so that the roles of these components are better segregated, but that is too big a change to block fixing this issue. --- backend/local/backend_plan.go | 32 +++++++++++++++++++++++++------- command/show.go | 13 +++++++++++-- command/show_test.go | 8 +++++++- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go index 0a7f28dac110..247076d3157a 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -8,6 +8,9 @@ import ( "sort" "strings" + "github.com/mitchellh/cli" + "github.com/mitchellh/colorstring" + "github.com/hashicorp/terraform/addrs" "github.com/hashicorp/terraform/backend" "github.com/hashicorp/terraform/command/format" @@ -190,6 +193,21 @@ func (b *Local) opPlan( } func (b *Local) renderPlan(plan *plans.Plan, state *states.State, schemas *terraform.Schemas) { + RenderPlan(plan, state, schemas, b.CLI, b.Colorize()) +} + +// RenderPlan renders the given plan to the given UI. +// +// This is exported only so that the "terraform show" command can re-use it. +// Ideally it would be somewhere outside of this backend code so that both +// can call into it, but we're leaving it here for now in order to avoid +// disruptive refactoring. +// +// If you find yourself wanting to call this function from a third callsite, +// please consider whether it's time to do the more disruptive refactoring +// so that something other than the local backend package is offering this +// functionality. +func RenderPlan(plan *plans.Plan, state *states.State, schemas *terraform.Schemas, ui cli.Ui, colorize *colorstring.Colorize) { counts := map[plans.Action]int{} var rChanges []*plans.ResourceInstanceChangeSrc for _, change := range plan.Changes.Resources { @@ -223,9 +241,9 @@ func (b *Local) renderPlan(plan *plans.Plan, state *states.State, schemas *terra fmt.Fprintf(headerBuf, "%s read (data resources)\n", format.DiffActionSymbol(plans.Read)) } - b.CLI.Output(b.Colorize().Color(headerBuf.String())) + ui.Output(colorize.Color(headerBuf.String())) - b.CLI.Output("Terraform will perform the following actions:\n") + ui.Output("Terraform will perform the following actions:\n") // Note: we're modifying the backing slice of this plan object in-place // here. The ordering of resource changes in a plan is not significant, @@ -247,13 +265,13 @@ func (b *Local) renderPlan(plan *plans.Plan, state *states.State, schemas *terra providerSchema := schemas.ProviderSchema(rcs.ProviderAddr.ProviderConfig.Type) if providerSchema == nil { // Should never happen - b.CLI.Output(fmt.Sprintf("(schema missing for %s)\n", rcs.ProviderAddr)) + ui.Output(fmt.Sprintf("(schema missing for %s)\n", rcs.ProviderAddr)) continue } rSchema, _ := providerSchema.SchemaForResourceAddr(rcs.Addr.Resource.Resource) if rSchema == nil { // Should never happen - b.CLI.Output(fmt.Sprintf("(schema missing for %s)\n", rcs.Addr)) + ui.Output(fmt.Sprintf("(schema missing for %s)\n", rcs.Addr)) continue } @@ -267,11 +285,11 @@ func (b *Local) renderPlan(plan *plans.Plan, state *states.State, schemas *terra } } - b.CLI.Output(format.ResourceChange( + ui.Output(format.ResourceChange( rcs, tainted, rSchema, - b.CLIColor, + colorize, )) } @@ -288,7 +306,7 @@ func (b *Local) renderPlan(plan *plans.Plan, state *states.State, schemas *terra stats[change.Action]++ } } - b.CLI.Output(b.Colorize().Color(fmt.Sprintf( + ui.Output(colorize.Color(fmt.Sprintf( "[reset][bold]Plan:[reset] "+ "%d to add, %d to change, %d to destroy.", stats[plans.Create], stats[plans.Update], stats[plans.Delete], diff --git a/command/show.go b/command/show.go index 2c75f930183d..d7b5f7ef837d 100644 --- a/command/show.go +++ b/command/show.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/hashicorp/terraform/backend" + localBackend "github.com/hashicorp/terraform/backend/local" "github.com/hashicorp/terraform/command/format" "github.com/hashicorp/terraform/command/jsonplan" "github.com/hashicorp/terraform/command/jsonstate" @@ -152,8 +153,16 @@ func (c *ShowCommand) Run(args []string) int { c.Ui.Output(string(jsonPlan)) return 0 } - dispPlan := format.NewPlan(plan.Changes) - c.Ui.Output(dispPlan.Format(c.Colorize())) + + // FIXME: We currently call into the local backend for this, since + // the "terraform plan" logic lives there and our package call graph + // means we can't orient this dependency the other way around. In + // future we'll hopefully be able to refactor the backend architecture + // a little so that CLI UI rendering always happens in this "command" + // package rather than in the backends themselves, but for now we're + // accepting this oddity because "terraform show" is a less commonly + // used way to render a plan than "terraform plan" is. + localBackend.RenderPlan(plan, stateFile.State, schemas, c.Ui, c.Colorize()) return 0 } diff --git a/command/show_test.go b/command/show_test.go index b66a1716886f..30da8c572482 100644 --- a/command/show_test.go +++ b/command/show_test.go @@ -79,7 +79,7 @@ func TestShow_noArgsNoState(t *testing.T) { func TestShow_plan(t *testing.T) { planPath := testPlanFileNoop(t) - ui := new(cli.MockUi) + ui := cli.NewMockUi() c := &ShowCommand{ Meta: Meta{ testingOverrides: metaOverridesForProvider(testProvider()), @@ -93,6 +93,12 @@ func TestShow_plan(t *testing.T) { if code := c.Run(args); code != 0 { t.Fatalf("bad: \n%s", ui.ErrorWriter.String()) } + + want := `Terraform will perform the following actions` + got := ui.OutputWriter.String() + if !strings.Contains(got, want) { + t.Errorf("missing expected output\nwant: %s\ngot:\n%s", want, got) + } } func TestShow_plan_json(t *testing.T) { From 807feb18472a5cb52b1045ea79745c08095d37a3 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 5 Nov 2019 17:22:49 -0800 Subject: [PATCH 2/2] command/format: Remove defunct "Plan" type and associated symbols This "Plan" type, along with the other types it directly or indirectly embeds and the associated functions, are adaptations of the flatmap-oriented plan renderer logic from Terraform 0.11 and prior. The current diff rendering logic is in diff.go, and so the contents of the plan.go file are defunct apart from the DiffActionSymbol function that both implementations share. Therefore here we move DiffActionSymbol into diff.go and then remove plan.go entirely, in the interests of dead code removal. --- command/format/diff.go | 23 ++++ command/format/plan.go | 306 ----------------------------------------- 2 files changed, 23 insertions(+), 306 deletions(-) delete mode 100644 command/format/plan.go diff --git a/command/format/diff.go b/command/format/diff.go index c726f0edeb31..f28b4f2b126f 100644 --- a/command/format/diff.go +++ b/command/format/diff.go @@ -1190,3 +1190,26 @@ func ctyNullBlockSetAsEmpty(in cty.Value) cty.Value { // sets, so our result here is always a set. return cty.SetValEmpty(in.Type().ElementType()) } + +// DiffActionSymbol returns a string that, once passed through a +// colorstring.Colorize, will produce a result that can be written +// to a terminal to produce a symbol made of three printable +// characters, possibly interspersed with VT100 color codes. +func DiffActionSymbol(action plans.Action) string { + switch action { + case plans.DeleteThenCreate: + return "[red]-[reset]/[green]+[reset]" + case plans.CreateThenDelete: + return "[green]+[reset]/[red]-[reset]" + case plans.Create: + return " [green]+[reset]" + case plans.Delete: + return " [red]-[reset]" + case plans.Read: + return " [cyan]<=[reset]" + case plans.Update: + return " [yellow]~[reset]" + default: + return " ?" + } +} diff --git a/command/format/plan.go b/command/format/plan.go deleted file mode 100644 index ef129a9289db..000000000000 --- a/command/format/plan.go +++ /dev/null @@ -1,306 +0,0 @@ -package format - -import ( - "bytes" - "fmt" - "log" - "sort" - "strings" - - "github.com/mitchellh/colorstring" - - "github.com/hashicorp/terraform/addrs" - "github.com/hashicorp/terraform/plans" - "github.com/hashicorp/terraform/states" - "github.com/hashicorp/terraform/terraform" -) - -// Plan is a representation of a plan optimized for display to -// an end-user, as opposed to terraform.Plan which is for internal use. -// -// DisplayPlan excludes implementation details that may otherwise appear -// in the main plan, such as destroy actions on data sources (which are -// there only to clean up the state). -type Plan struct { - Resources []*InstanceDiff -} - -// InstanceDiff is a representation of an instance diff optimized -// for display, in conjunction with DisplayPlan. -type InstanceDiff struct { - Addr *terraform.ResourceAddress - Action plans.Action - - // Attributes describes changes to the attributes of the instance. - // - // For destroy diffs this is always nil. - Attributes []*AttributeDiff - - Tainted bool - Deposed bool -} - -// AttributeDiff is a representation of an attribute diff optimized -// for display, in conjunction with DisplayInstanceDiff. -type AttributeDiff struct { - // Path is a dot-delimited traversal through possibly many levels of list and map structure, - // intended for display purposes only. - Path string - - Action plans.Action - - OldValue string - NewValue string - - NewComputed bool - Sensitive bool - ForcesNew bool -} - -// PlanStats gives summary counts for a Plan. -type PlanStats struct { - ToAdd, ToChange, ToDestroy int -} - -// NewPlan produces a display-oriented Plan from a terraform.Plan. -func NewPlan(changes *plans.Changes) *Plan { - log.Printf("[TRACE] NewPlan for %#v", changes) - ret := &Plan{} - if changes == nil { - // Nothing to do! - return ret - } - - for _, rc := range changes.Resources { - addr := rc.Addr - log.Printf("[TRACE] NewPlan found %s (%s)", addr, rc.Action) - dataSource := addr.Resource.Resource.Mode == addrs.DataResourceMode - - // We create "delete" actions for data resources so we can clean - // up their entries in state, but this is an implementation detail - // that users shouldn't see. - if dataSource && rc.Action == plans.Delete { - continue - } - - if rc.Action == plans.NoOp { - continue - } - - // For now we'll shim this to work with our old types. - // TODO: Update for the new plan types, ideally also switching over to - // a structural diff renderer instead of a flat renderer. - did := &InstanceDiff{ - Addr: terraform.NewLegacyResourceInstanceAddress(addr), - Action: rc.Action, - } - - if rc.DeposedKey != states.NotDeposed { - did.Deposed = true - } - - // Since this is just a temporary stub implementation on the way - // to us replacing this with the structural diff renderer, we currently - // don't include any attributes here. - // FIXME: Implement the structural diff renderer to replace this - // codepath altogether. - - ret.Resources = append(ret.Resources, did) - } - - // Sort the instance diffs by their addresses for display. - sort.Slice(ret.Resources, func(i, j int) bool { - iAddr := ret.Resources[i].Addr - jAddr := ret.Resources[j].Addr - return iAddr.Less(jAddr) - }) - - return ret -} - -// Format produces and returns a text representation of the receiving plan -// intended for display in a terminal. -// -// If color is not nil, it is used to colorize the output. -func (p *Plan) Format(color *colorstring.Colorize) string { - if p.Empty() { - return "This plan does nothing." - } - - if color == nil { - color = &colorstring.Colorize{ - Colors: colorstring.DefaultColors, - Reset: false, - } - } - - // Find the longest path length of all the paths that are changing, - // so we can align them all. - keyLen := 0 - for _, r := range p.Resources { - for _, attr := range r.Attributes { - key := attr.Path - - if len(key) > keyLen { - keyLen = len(key) - } - } - } - - buf := new(bytes.Buffer) - for _, r := range p.Resources { - formatPlanInstanceDiff(buf, r, keyLen, color) - } - - return strings.TrimSpace(buf.String()) -} - -// Stats returns statistics about the plan -func (p *Plan) Stats() PlanStats { - var ret PlanStats - for _, r := range p.Resources { - switch r.Action { - case plans.Create: - ret.ToAdd++ - case plans.Update: - ret.ToChange++ - case plans.DeleteThenCreate, plans.CreateThenDelete: - ret.ToAdd++ - ret.ToDestroy++ - case plans.Delete: - ret.ToDestroy++ - } - } - return ret -} - -// ActionCounts returns the number of diffs for each action type -func (p *Plan) ActionCounts() map[plans.Action]int { - ret := map[plans.Action]int{} - for _, r := range p.Resources { - ret[r.Action]++ - } - return ret -} - -// Empty returns true if there is at least one resource diff in the receiving plan. -func (p *Plan) Empty() bool { - return len(p.Resources) == 0 -} - -// DiffActionSymbol returns a string that, once passed through a -// colorstring.Colorize, will produce a result that can be written -// to a terminal to produce a symbol made of three printable -// characters, possibly interspersed with VT100 color codes. -func DiffActionSymbol(action plans.Action) string { - switch action { - case plans.DeleteThenCreate: - return "[red]-[reset]/[green]+[reset]" - case plans.CreateThenDelete: - return "[green]+[reset]/[red]-[reset]" - case plans.Create: - return " [green]+[reset]" - case plans.Delete: - return " [red]-[reset]" - case plans.Read: - return " [cyan]<=[reset]" - case plans.Update: - return " [yellow]~[reset]" - default: - return " ?" - } -} - -// formatPlanInstanceDiff writes the text representation of the given instance diff -// to the given buffer, using the given colorizer. -func formatPlanInstanceDiff(buf *bytes.Buffer, r *InstanceDiff, keyLen int, colorizer *colorstring.Colorize) { - addrStr := r.Addr.String() - - // Determine the color for the text (green for adding, yellow - // for change, red for delete), and symbol, and output the - // resource header. - color := "yellow" - symbol := DiffActionSymbol(r.Action) - oldValues := true - switch r.Action { - case plans.DeleteThenCreate, plans.CreateThenDelete: - color = "yellow" - case plans.Create: - color = "green" - oldValues = false - case plans.Delete: - color = "red" - case plans.Read: - color = "cyan" - oldValues = false - } - - var extraStr string - if r.Tainted { - extraStr = extraStr + " (tainted)" - } - if r.Deposed { - extraStr = extraStr + " (deposed)" - } - if r.Action.IsReplace() { - extraStr = extraStr + colorizer.Color(" [red][bold](new resource required)") - } - - buf.WriteString( - colorizer.Color(fmt.Sprintf( - "[%s]%s [%s]%s%s\n", - color, symbol, color, addrStr, extraStr, - )), - ) - - for _, attr := range r.Attributes { - - v := attr.NewValue - var dispV string - switch { - case v == "" && attr.NewComputed: - dispV = "" - case attr.Sensitive: - dispV = "" - default: - dispV = fmt.Sprintf("%q", v) - } - - updateMsg := "" - switch { - case attr.ForcesNew && r.Action.IsReplace(): - updateMsg = colorizer.Color(" [red](forces new resource)") - case attr.Sensitive && oldValues: - updateMsg = colorizer.Color(" [yellow](attribute changed)") - } - - if oldValues { - u := attr.OldValue - var dispU string - switch { - case attr.Sensitive: - dispU = "" - default: - dispU = fmt.Sprintf("%q", u) - } - buf.WriteString(fmt.Sprintf( - " %s:%s %s => %s%s\n", - attr.Path, - strings.Repeat(" ", keyLen-len(attr.Path)), - dispU, dispV, - updateMsg, - )) - } else { - buf.WriteString(fmt.Sprintf( - " %s:%s %s%s\n", - attr.Path, - strings.Repeat(" ", keyLen-len(attr.Path)), - dispV, - updateMsg, - )) - } - } - - // Write the reset color so we don't bleed color into later text - buf.WriteString(colorizer.Color("[reset]\n")) -}