From ac16c12ec41d2c43672df7f9d9980bbd62ae9652 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 22 Aug 2017 18:36:17 -0700 Subject: [PATCH 1/7] core: stabilize ResourceAddress.Less results The implementation of ResourceAddress.Less was flawed because it was only testing each field in the "less than" direction, and falling through in cases where an earlier field compared greater than a later one. Now we test for inequality first as the selector, and only fall through if the two values for a given field are equal. --- terraform/resource_address.go | 31 +++++++++++++++--------------- terraform/resource_address_test.go | 20 +++++++++++++++++++ 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/terraform/resource_address.go b/terraform/resource_address.go index 8badca8053c6..353725844556 100644 --- a/terraform/resource_address.go +++ b/terraform/resource_address.go @@ -362,40 +362,41 @@ func (addr *ResourceAddress) Less(other *ResourceAddress) bool { switch { - case len(addr.Path) < len(other.Path): - return true + case len(addr.Path) != len(other.Path): + return len(addr.Path) < len(other.Path) case !reflect.DeepEqual(addr.Path, other.Path): // If the two paths are the same length but don't match, we'll just // cheat and compare the string forms since it's easier than - // comparing all of the path segments in turn. + // comparing all of the path segments in turn, and lexicographic + // comparison is correct for the module path portion. addrStr := addr.String() otherStr := other.String() return addrStr < otherStr - case addr.Mode == config.DataResourceMode && other.Mode != config.DataResourceMode: - return true + case addr.Mode != other.Mode: + return addr.Mode == config.DataResourceMode - case addr.Type < other.Type: - return true + case addr.Type != other.Type: + return addr.Type < other.Type - case addr.Name < other.Name: - return true + case addr.Name != other.Name: + return addr.Name < other.Name - case addr.Index < other.Index: + case addr.Index != other.Index: // Since "Index" is -1 for an un-indexed address, this also conveniently // sorts unindexed addresses before indexed ones, should they both // appear for some reason. - return true + return addr.Index < other.Index - case other.InstanceTypeSet && !addr.InstanceTypeSet: - return true + case addr.InstanceTypeSet != other.InstanceTypeSet: + return !addr.InstanceTypeSet - case addr.InstanceType < other.InstanceType: + case addr.InstanceType != other.InstanceType: // InstanceType is actually an enum, so this is just an arbitrary // sort based on the enum numeric values, and thus not particularly // meaningful. - return true + return addr.InstanceType < other.InstanceType default: return false diff --git a/terraform/resource_address_test.go b/terraform/resource_address_test.go index a2f9cb9ff068..0fe561528834 100644 --- a/terraform/resource_address_test.go +++ b/terraform/resource_address_test.go @@ -1233,6 +1233,11 @@ func TestResourceAddressLess(t *testing.T) { "module.baz.foo.bar", true, }, + { + "module.baz.foo.bar", + "zzz.bar", // would sort after "module" in lexicographical sort + false, + }, { "module.baz.foo.bar", "module.baz.foo.bar", @@ -1243,6 +1248,11 @@ func TestResourceAddressLess(t *testing.T) { "module.boz.foo.bar", true, }, + { + "module.boz.foo.bar", + "module.baz.foo.bar", + false, + }, { "a.b", "b.c", @@ -1253,11 +1263,21 @@ func TestResourceAddressLess(t *testing.T) { "a.c", true, }, + { + "c.b", + "b.c", + false, + }, { "a.b[9]", "a.b[10]", true, }, + { + "b.b[9]", + "a.b[10]", + false, + }, { "a.b", "a.b.deposed", From f1807a7eee7fdf55fd6f479195b2cefeadfa57e4 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 23 Aug 2017 16:23:02 -0700 Subject: [PATCH 2/7] command/format: improve consistency of plan results Previously the rendered plan output was constructed directly from the core plan and then annotated with counts derived from the count hook. At various places we applied little adjustments to deal with the fact that the user-facing diff model is not identical to the internal diff model, including the special handling of data source reads and destroys. Since this logic was just muddled into the rendering code, it behaved inconsistently with the tally of adds, updates and deletes. This change reworks the plan formatter so that it happens in two stages: - First, we produce a specialized Plan object that is tailored for use in the UI. This applies all the relevant logic to transform the physical model into the user model. - Second, we do a straightforward visual rendering of the display-oriented plan object. For the moment this is slightly overkill since there's only one rendering path, but it does give us the benefit of letting the counts be derived from the same data as the full detailed diff, ensuring that they'll stay consistent. Later we may choose to have other UIs for plans, such as a machine-readable output intended to drive a web UI. In that case, we'd want the web UI to consume a serialization of the _display-oriented_ plan so that it doesn't need to re-implement all of these UI special cases. This introduces to core a new diff action type for "refresh". Currently this is used _only_ in the UI layer, to represent data source reads. Later it would be good to use this type for the core diff as well, to improve consistency, but that is left for another day to keep this change focused on the UI. --- backend/local/backend_apply.go | 28 +- backend/local/backend_plan.go | 15 +- command/format/plan.go | 441 +++++++++++++++++------------ command/format/plan_test.go | 496 ++++++++++++++++++++++++++++++--- command/show.go | 7 +- terraform/diff.go | 6 + 6 files changed, 737 insertions(+), 256 deletions(-) diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index 9b6c7e3a44e4..608c44411750 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -96,28 +96,16 @@ func (b *Local) opApply( return } - trivialPlan := plan.Diff == nil || plan.Diff.Empty() + dispPlan := format.NewPlan(plan) + trivialPlan := dispPlan.Empty() hasUI := op.UIOut != nil && op.UIIn != nil - if hasUI && ((op.Destroy && !op.DestroyForce) || - (!op.Destroy && !op.AutoApprove && !trivialPlan)) { + mustConfirm := hasUI && ((op.Destroy && !op.DestroyForce) || (!op.Destroy && !op.AutoApprove && !trivialPlan)) + if mustConfirm { var desc, query string if op.Destroy { // Default destroy message - desc = "Terraform will delete all your managed infrastructure, as shown above.\n" + + desc = "Terraform will destroy all your managed infrastructure, as shown above.\n" + "There is no undo. Only 'yes' will be accepted to confirm." - - // If targets are specified, list those to user - if op.Targets != nil { - var descBuffer bytes.Buffer - descBuffer.WriteString("Terraform will delete the following infrastructure:\n") - for _, target := range op.Targets { - descBuffer.WriteString("\t") - descBuffer.WriteString(target) - descBuffer.WriteString("\n") - } - descBuffer.WriteString("There is no undo. Only 'yes' will be accepted to confirm") - desc = descBuffer.String() - } query = "Do you really want to destroy?" } else { desc = "Terraform will apply the changes described above.\n" + @@ -132,11 +120,7 @@ func (b *Local) opApply( } else { op.UIOut.Output("\n" + strings.TrimSpace(approvePlanHeader) + "\n") } - op.UIOut.Output(format.Plan(&format.PlanOpts{ - Plan: plan, - Color: b.Colorize(), - ModuleDepth: -1, - })) + op.UIOut.Output(dispPlan.Format(b.Colorize())) } v, err := op.UIIn.Input(&terraform.InputOpts{ diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go index 42a56eb291c9..83d1733dec76 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -133,7 +133,8 @@ func (b *Local) opPlan( // Perform some output tasks if we have a CLI to output to. if b.CLI != nil { - if plan.Diff.Empty() { + dispPlan := format.NewPlan(plan) + if dispPlan.Empty() { b.CLI.Output(b.Colorize().Color(strings.TrimSpace(planNoChanges))) return } @@ -146,18 +147,14 @@ func (b *Local) opPlan( path)) } - b.CLI.Output(format.Plan(&format.PlanOpts{ - Plan: plan, - Color: b.Colorize(), - ModuleDepth: -1, - })) + b.CLI.Output(dispPlan.Format(b.Colorize())) + stats := dispPlan.Stats() b.CLI.Output(b.Colorize().Color(fmt.Sprintf( "[reset][bold]Plan:[reset] "+ "%d to add, %d to change, %d to destroy.", - countHook.ToAdd+countHook.ToRemoveAndAdd, - countHook.ToChange, - countHook.ToRemove+countHook.ToRemoveAndAdd))) + stats.ToAdd, stats.ToChange, stats.ToDestroy, + ))) } } diff --git a/command/format/plan.go b/command/format/plan.go index 6bc699e398bd..1220b85a5e7d 100644 --- a/command/format/plan.go +++ b/command/format/plan.go @@ -11,232 +11,315 @@ import ( "github.com/mitchellh/colorstring" ) -// PlanOpts are the options for formatting a plan. -type PlanOpts struct { - // Plan is the plan to format. This is required. - Plan *terraform.Plan +// 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 terraform.DiffChangeType - // Color is the colorizer. This is optional. - Color *colorstring.Colorize + // Attributes describes changes to the attributes of the instance. + // + // For destroy diffs this is always nil. + Attributes []*AttributeDiff - // ModuleDepth is the depth of the modules to expand. By default this - // is zero which will not expand modules at all. - ModuleDepth int + Tainted bool + Deposed bool } -// Plan takes a plan and returns a -func Plan(opts *PlanOpts) string { - p := opts.Plan - if p.Diff == nil || p.Diff.Empty() { - return "This plan does nothing." - } +// 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 - if opts.Color == nil { - opts.Color = &colorstring.Colorize{ - Colors: colorstring.DefaultColors, - Reset: false, - } - } + Action terraform.DiffChangeType - buf := new(bytes.Buffer) - for _, m := range p.Diff.Modules { - if len(m.Path)-1 <= opts.ModuleDepth || opts.ModuleDepth == -1 { - formatPlanModuleExpand(buf, m, opts) - } else { - formatPlanModuleSingle(buf, m, opts) - } - } + OldValue string + NewValue string - return strings.TrimSpace(buf.String()) + NewComputed bool + Sensitive bool + ForcesNew bool } -// formatPlanModuleExpand will output the given module and all of its -// resources. -func formatPlanModuleExpand( - buf *bytes.Buffer, m *terraform.ModuleDiff, opts *PlanOpts) { - // Ignore empty diffs - if m.Empty() { - return - } +// PlanStats gives summary counts for a Plan. +type PlanStats struct { + ToAdd, ToChange, ToDestroy int +} - var modulePath []string - if !m.IsRoot() { - modulePath = m.Path[1:] +// NewPlan produces a display-oriented Plan from a terraform.Plan. +func NewPlan(plan *terraform.Plan) *Plan { + ret := &Plan{} + if plan == nil || plan.Diff == nil || plan.Diff.Empty() { + // Nothing to do! + return ret } - // We want to output the resources in sorted order to make things - // easier to scan through, so get all the resource names and sort them. - names := make([]string, 0, len(m.Resources)) - addrs := map[string]*terraform.ResourceAddress{} - for name := range m.Resources { - names = append(names, name) - var err error - addrs[name], err = terraform.ParseResourceAddressForInstanceDiff(modulePath, name) - if err != nil { - // should never happen; indicates invalid diff - panic("invalid resource address in diff") + for _, m := range plan.Diff.Modules { + var modulePath []string + if !m.IsRoot() { + // trim off the leading "root" path segment, since it's implied + // when we use a path in a resource address. + modulePath = m.Path[1:] } - } - sort.Slice(names, func(i, j int) bool { - return addrs[names[i]].Less(addrs[names[j]]) - }) - // Go through each sorted name and start building the output - for _, name := range names { - rdiff := m.Resources[name] - if rdiff.Empty() { - continue - } + for k, r := range m.Resources { + if r.Empty() { + continue + } - addr := addrs[name] - addrStr := addr.String() - dataSource := addr.Mode == config.DataResourceMode - - // 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 := " ~" - oldValues := true - switch rdiff.ChangeType() { - case terraform.DiffDestroyCreate: - color = "yellow" - symbol = "[red]-[reset]/[green]+[reset][yellow]" - case terraform.DiffCreate: - color = "green" - symbol = " +" - oldValues = false - - // If we're "creating" a data resource then we'll present it - // to the user as a "read" operation, so it's clear that this - // operation won't change anything outside of the Terraform state. - // Unfortunately by the time we get here we only have the name - // to work with, so we need to cheat and exploit knowledge of the - // naming scheme for data resources. - if dataSource { - symbol = " <=" - color = "cyan" + addr, err := terraform.ParseResourceAddressForInstanceDiff(modulePath, k) + if err != nil { + // should never happen; indicates invalid diff + panic("invalid resource address in diff") } - case terraform.DiffDestroy: - color = "red" - symbol = " -" - } - var extraAttr []string - if rdiff.DestroyTainted { - extraAttr = append(extraAttr, "tainted") - } - if rdiff.DestroyDeposed { - extraAttr = append(extraAttr, "deposed") - } - var extraStr string - if len(extraAttr) > 0 { - extraStr = fmt.Sprintf(" (%s)", strings.Join(extraAttr, ", ")) - } - if rdiff.ChangeType() == terraform.DiffDestroyCreate { - extraStr = extraStr + opts.Color.Color(" [red][bold](new resource required)") - } + dataSource := addr.Mode == config.DataResourceMode - buf.WriteString(opts.Color.Color(fmt.Sprintf( - "[%s]%s %s%s\n", - color, symbol, addrStr, extraStr))) - - // Get all the attributes that are changing, and sort them. Also - // determine the longest key so that we can align them all. - keyLen := 0 - keys := make([]string, 0, len(rdiff.Attributes)) - for key, _ := range rdiff.Attributes { - // Skip the ID since we do that specially - if key == "id" { + // We create "destroy" 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 && r.ChangeType() == terraform.DiffDestroy { continue } - keys = append(keys, key) - if len(key) > keyLen { - keyLen = len(key) + did := &InstanceDiff{ + Addr: addr, + Action: r.ChangeType(), + Tainted: r.DestroyTainted, + Deposed: r.DestroyDeposed, } - } - sort.Strings(keys) - - // Go through and output each attribute - for _, attrK := range keys { - attrDiff := rdiff.Attributes[attrK] - v := attrDiff.New - if v == "" && attrDiff.NewComputed { - v = "" + if dataSource && did.Action == terraform.DiffCreate { + // Use "refresh" as the action for display, since core + // currently uses Create for this. + did.Action = terraform.DiffRefresh } - if attrDiff.Sensitive { - v = "" + ret.Resources = append(ret.Resources, did) + + if did.Action == terraform.DiffDestroy { + // Don't show any outputs for destroy actions + continue } - updateMsg := "" - if attrDiff.RequiresNew && rdiff.Destroy { - updateMsg = opts.Color.Color(" [red](forces new resource)") - } else if attrDiff.Sensitive && oldValues { - updateMsg = opts.Color.Color(" [yellow](attribute changed)") + for k, a := range r.Attributes { + var action terraform.DiffChangeType + switch { + case a.NewRemoved: + action = terraform.DiffDestroy + case did.Action == terraform.DiffCreate: + action = terraform.DiffCreate + default: + action = terraform.DiffUpdate + } + + did.Attributes = append(did.Attributes, &AttributeDiff{ + Path: k, + Action: action, + + OldValue: a.Old, + NewValue: a.New, + + Sensitive: a.Sensitive, + ForcesNew: a.RequiresNew, + NewComputed: a.NewComputed, + }) } - if oldValues { - var u string - if attrDiff.Sensitive { - u = "" - } else { - u = attrDiff.Old + // Sort the attributes by their paths for display + sort.Slice(did.Attributes, func(i, j int) bool { + iPath := did.Attributes[i].Path + jPath := did.Attributes[j].Path + + // as a special case, "id" is always first + switch { + case iPath != jPath && (iPath == "id" || jPath == "id"): + return iPath == "id" + default: + return iPath < jPath } - buf.WriteString(fmt.Sprintf( - " %s:%s %#v => %#v%s\n", - attrK, - strings.Repeat(" ", keyLen-len(attrK)), - u, - v, - updateMsg)) - } else { - buf.WriteString(fmt.Sprintf( - " %s:%s %#v%s\n", - attrK, - strings.Repeat(" ", keyLen-len(attrK)), - v, - updateMsg)) + }) + + } + } + + // 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) } } + } - // Write the reset color so we don't overload the user's terminal - buf.WriteString(opts.Color.Color("[reset]\n")) + buf := new(bytes.Buffer) + for _, r := range p.Resources { + formatPlanInstanceDiff(buf, r, keyLen, color) } + + return strings.TrimSpace(buf.String()) } -// formatPlanModuleSingle will output the given module and all of its -// resources. -func formatPlanModuleSingle( - buf *bytes.Buffer, m *terraform.ModuleDiff, opts *PlanOpts) { - // Ignore empty diffs - if m.Empty() { - return +// Stats returns statistics about the plan +func (p *Plan) Stats() PlanStats { + var ret PlanStats + for _, r := range p.Resources { + switch r.Action { + case terraform.DiffCreate: + ret.ToAdd++ + case terraform.DiffUpdate: + ret.ToChange++ + case terraform.DiffDestroyCreate: + ret.ToAdd++ + ret.ToDestroy++ + case terraform.DiffDestroy: + ret.ToDestroy++ + } } + return ret +} - moduleName := fmt.Sprintf("module.%s", strings.Join(m.Path[1:], ".")) +// 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 +} + +// 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 := "~" - switch m.ChangeType() { + symbol := " ~" + oldValues := true + switch r.Action { + case terraform.DiffDestroyCreate: + color = "yellow" + symbol = "[red]-[reset]/[green]+[reset][yellow]" case terraform.DiffCreate: color = "green" - symbol = "+" + symbol = " +" + oldValues = false case terraform.DiffDestroy: color = "red" - symbol = "-" + symbol = " -" + case terraform.DiffRefresh: + symbol = " <=" + color = "cyan" + oldValues = false + } + + var extraStr string + if r.Tainted { + extraStr = extraStr + colorizer.Color(" (tainted)") + } + if r.Deposed { + extraStr = extraStr + colorizer.Color(" (deposed)") + } + if r.Action == terraform.DiffDestroyCreate { + extraStr = extraStr + colorizer.Color(" [red][bold](new resource required)") + } + + buf.WriteString( + colorizer.Color(fmt.Sprintf( + "[%s]%s %s%s\n", + color, symbol, 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 == terraform.DiffDestroy: + 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, + )) + } } - buf.WriteString(opts.Color.Color(fmt.Sprintf( - "[%s]%s %s\n", - color, symbol, moduleName))) - buf.WriteString(fmt.Sprintf( - " %d resource(s)", - len(m.Resources))) - buf.WriteString(opts.Color.Color("[reset]\n")) + // Write the reset color so we don't bleed color into later text + buf.WriteString(colorizer.Color("[reset]\n")) } diff --git a/command/format/plan_test.go b/command/format/plan_test.go index 82390d3d9d31..7ee3802617e2 100644 --- a/command/format/plan_test.go +++ b/command/format/plan_test.go @@ -1,14 +1,452 @@ package format import ( + "reflect" "strings" "testing" + "github.com/davecgh/go-spew/spew" "github.com/hashicorp/terraform/terraform" "github.com/mitchellh/colorstring" ) -// Test that a root level data source gets a special plan output on create +var disabledColorize = &colorstring.Colorize{ + Colors: colorstring.DefaultColors, + Disable: true, +} + +func TestNewPlan(t *testing.T) { + tests := map[string]struct { + Input *terraform.Plan + Want *Plan + }{ + "nil input": { + Input: nil, + Want: &Plan{ + Resources: nil, + }, + }, + "nil diff": { + Input: &terraform.Plan{}, + Want: &Plan{ + Resources: nil, + }, + }, + "empty diff": { + Input: &terraform.Plan{ + Diff: &terraform.Diff{ + Modules: []*terraform.ModuleDiff{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.InstanceDiff{}, + }, + }, + }, + }, + Want: &Plan{ + Resources: nil, + }, + }, + "create managed resource": { + Input: &terraform.Plan{ + Diff: &terraform.Diff{ + Modules: []*terraform.ModuleDiff{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.InstanceDiff{ + "test_resource.foo": { + Attributes: map[string]*terraform.ResourceAttrDiff{ + "id": { + NewComputed: true, + RequiresNew: true, + }, + }, + }, + }, + }, + }, + }, + }, + Want: &Plan{ + Resources: []*InstanceDiff{ + { + Addr: mustParseResourceAddress("test_resource.foo"), + Action: terraform.DiffCreate, + Attributes: []*AttributeDiff{ + { + Path: "id", + Action: terraform.DiffCreate, + NewComputed: true, + ForcesNew: true, + }, + }, + }, + }, + }, + }, + "create managed resource in child module": { + Input: &terraform.Plan{ + Diff: &terraform.Diff{ + Modules: []*terraform.ModuleDiff{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.InstanceDiff{ + "test_resource.foo": { + Attributes: map[string]*terraform.ResourceAttrDiff{ + "id": { + NewComputed: true, + RequiresNew: true, + }, + }, + }, + }, + }, + { + Path: []string{"root", "foo"}, + Resources: map[string]*terraform.InstanceDiff{ + "test_resource.foo": { + Attributes: map[string]*terraform.ResourceAttrDiff{ + "id": { + NewComputed: true, + RequiresNew: true, + }, + }, + }, + }, + }, + }, + }, + }, + Want: &Plan{ + Resources: []*InstanceDiff{ + { + Addr: mustParseResourceAddress("test_resource.foo"), + Action: terraform.DiffCreate, + Attributes: []*AttributeDiff{ + { + Path: "id", + Action: terraform.DiffCreate, + NewComputed: true, + ForcesNew: true, + }, + }, + }, + { + Addr: mustParseResourceAddress("module.foo.test_resource.foo"), + Action: terraform.DiffCreate, + Attributes: []*AttributeDiff{ + { + Path: "id", + Action: terraform.DiffCreate, + NewComputed: true, + ForcesNew: true, + }, + }, + }, + }, + }, + }, + "create data resource": { + Input: &terraform.Plan{ + Diff: &terraform.Diff{ + Modules: []*terraform.ModuleDiff{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.InstanceDiff{ + "data.test_data_source.foo": { + Attributes: map[string]*terraform.ResourceAttrDiff{ + "id": { + NewComputed: true, + RequiresNew: true, + }, + }, + }, + }, + }, + }, + }, + }, + Want: &Plan{ + Resources: []*InstanceDiff{ + { + Addr: mustParseResourceAddress("data.test_data_source.foo"), + Action: terraform.DiffRefresh, + Attributes: []*AttributeDiff{ + { + Path: "id", + Action: terraform.DiffUpdate, + NewComputed: true, + ForcesNew: true, + }, + }, + }, + }, + }, + }, + "destroy managed resource": { + Input: &terraform.Plan{ + Diff: &terraform.Diff{ + Modules: []*terraform.ModuleDiff{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.InstanceDiff{ + "test_resource.foo": { + Destroy: true, + }, + }, + }, + }, + }, + }, + Want: &Plan{ + Resources: []*InstanceDiff{ + { + Addr: mustParseResourceAddress("test_resource.foo"), + Action: terraform.DiffDestroy, + }, + }, + }, + }, + "destroy data resource": { + Input: &terraform.Plan{ + Diff: &terraform.Diff{ + Modules: []*terraform.ModuleDiff{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.InstanceDiff{ + "data.test_data_source.foo": { + Destroy: true, + }, + }, + }, + }, + }, + }, + Want: &Plan{ + // Data source destroys are not shown + Resources: nil, + }, + }, + "destroy many instances of a resource": { + Input: &terraform.Plan{ + Diff: &terraform.Diff{ + Modules: []*terraform.ModuleDiff{ + { + Path: []string{"root"}, + Resources: map[string]*terraform.InstanceDiff{ + "test_resource.foo.0": { + Destroy: true, + }, + "test_resource.foo.1": { + Destroy: true, + }, + "test_resource.foo.10": { + Destroy: true, + }, + "test_resource.foo.2": { + Destroy: true, + }, + "test_resource.foo.3": { + Destroy: true, + }, + "test_resource.foo.4": { + Destroy: true, + }, + "test_resource.foo.5": { + Destroy: true, + }, + "test_resource.foo.6": { + Destroy: true, + }, + "test_resource.foo.7": { + Destroy: true, + }, + "test_resource.foo.8": { + Destroy: true, + }, + "test_resource.foo.9": { + Destroy: true, + }, + }, + }, + }, + }, + }, + Want: &Plan{ + Resources: []*InstanceDiff{ + { + Addr: mustParseResourceAddress("test_resource.foo[0]"), + Action: terraform.DiffDestroy, + }, + { + Addr: mustParseResourceAddress("test_resource.foo[1]"), + Action: terraform.DiffDestroy, + }, + { + Addr: mustParseResourceAddress("test_resource.foo[2]"), + Action: terraform.DiffDestroy, + }, + { + Addr: mustParseResourceAddress("test_resource.foo[3]"), + Action: terraform.DiffDestroy, + }, + { + Addr: mustParseResourceAddress("test_resource.foo[4]"), + Action: terraform.DiffDestroy, + }, + { + Addr: mustParseResourceAddress("test_resource.foo[5]"), + Action: terraform.DiffDestroy, + }, + { + Addr: mustParseResourceAddress("test_resource.foo[6]"), + Action: terraform.DiffDestroy, + }, + { + Addr: mustParseResourceAddress("test_resource.foo[7]"), + Action: terraform.DiffDestroy, + }, + { + Addr: mustParseResourceAddress("test_resource.foo[8]"), + Action: terraform.DiffDestroy, + }, + { + Addr: mustParseResourceAddress("test_resource.foo[9]"), + Action: terraform.DiffDestroy, + }, + { + Addr: mustParseResourceAddress("test_resource.foo[10]"), + Action: terraform.DiffDestroy, + }, + }, + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + got := NewPlan(test.Input) + if !reflect.DeepEqual(got, test.Want) { + t.Errorf( + "wrong result\ninput: %sgot: %swant:%s", + spew.Sdump(test.Input), + spew.Sdump(got), + spew.Sdump(test.Want), + ) + } + }) + } +} + +func TestPlanStats(t *testing.T) { + tests := map[string]struct { + Input *Plan + Want PlanStats + }{ + "empty": { + &Plan{}, + PlanStats{}, + }, + "destroy": { + &Plan{ + Resources: []*InstanceDiff{ + { + Addr: mustParseResourceAddress("test_resource.foo"), + Action: terraform.DiffDestroy, + }, + { + Addr: mustParseResourceAddress("test_resource.bar"), + Action: terraform.DiffDestroy, + }, + }, + }, + PlanStats{ + ToDestroy: 2, + }, + }, + "create": { + &Plan{ + Resources: []*InstanceDiff{ + { + Addr: mustParseResourceAddress("test_resource.foo"), + Action: terraform.DiffCreate, + }, + { + Addr: mustParseResourceAddress("test_resource.bar"), + Action: terraform.DiffCreate, + }, + }, + }, + PlanStats{ + ToAdd: 2, + }, + }, + "update": { + &Plan{ + Resources: []*InstanceDiff{ + { + Addr: mustParseResourceAddress("test_resource.foo"), + Action: terraform.DiffUpdate, + }, + { + Addr: mustParseResourceAddress("test_resource.bar"), + Action: terraform.DiffUpdate, + }, + }, + }, + PlanStats{ + ToChange: 2, + }, + }, + "data source refresh": { + &Plan{ + Resources: []*InstanceDiff{ + { + Addr: mustParseResourceAddress("data.test.foo"), + Action: terraform.DiffRefresh, + }, + }, + }, + PlanStats{ + // data resource refreshes are not counted in our stats + }, + }, + "replace": { + &Plan{ + Resources: []*InstanceDiff{ + { + Addr: mustParseResourceAddress("test_resource.foo"), + Action: terraform.DiffDestroyCreate, + }, + { + Addr: mustParseResourceAddress("test_resource.bar"), + Action: terraform.DiffDestroyCreate, + }, + }, + }, + PlanStats{ + ToDestroy: 2, + ToAdd: 2, + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + got := test.Input.Stats() + if !reflect.DeepEqual(got, test.Want) { + t.Errorf( + "wrong result\ninput: %sgot: %swant:%s", + spew.Sdump(test.Input), + spew.Sdump(got), + spew.Sdump(test.Want), + ) + } + }) + } +} + +// Test that deposed instances are marked as such func TestPlan_destroyDeposed(t *testing.T) { plan := &terraform.Plan{ Diff: &terraform.Diff{ @@ -24,16 +462,8 @@ func TestPlan_destroyDeposed(t *testing.T) { }, }, } - opts := &PlanOpts{ - Plan: plan, - Color: &colorstring.Colorize{ - Colors: colorstring.DefaultColors, - Disable: true, - }, - ModuleDepth: 1, - } - - actual := Plan(opts) + dispPlan := NewPlan(plan) + actual := dispPlan.Format(disabledColorize) expected := strings.TrimSpace(` - aws_instance.foo (deposed) @@ -64,16 +494,8 @@ func TestPlan_displayInterpolations(t *testing.T) { }, }, } - opts := &PlanOpts{ - Plan: plan, - Color: &colorstring.Colorize{ - Colors: colorstring.DefaultColors, - Disable: true, - }, - ModuleDepth: 1, - } - - out := Plan(opts) + dispPlan := NewPlan(plan) + out := dispPlan.Format(disabledColorize) lines := strings.Split(out, "\n") if len(lines) != 2 { t.Fatal("expected 2 lines of output, got:\n", out) @@ -108,16 +530,8 @@ func TestPlan_rootDataSource(t *testing.T) { }, }, } - opts := &PlanOpts{ - Plan: plan, - Color: &colorstring.Colorize{ - Colors: colorstring.DefaultColors, - Disable: true, - }, - ModuleDepth: 1, - } - - actual := Plan(opts) + dispPlan := NewPlan(plan) + actual := dispPlan.Format(disabledColorize) expected := strings.TrimSpace(` <= data.type.name @@ -149,16 +563,8 @@ func TestPlan_nestedDataSource(t *testing.T) { }, }, } - opts := &PlanOpts{ - Plan: plan, - Color: &colorstring.Colorize{ - Colors: colorstring.DefaultColors, - Disable: true, - }, - ModuleDepth: 2, - } - - actual := Plan(opts) + dispPlan := NewPlan(plan) + actual := dispPlan.Format(disabledColorize) expected := strings.TrimSpace(` <= module.nested.data.type.name @@ -168,3 +574,11 @@ func TestPlan_nestedDataSource(t *testing.T) { t.Fatalf("expected:\n\n%s\n\ngot:\n\n%s", expected, actual) } } + +func mustParseResourceAddress(s string) *terraform.ResourceAddress { + addr, err := terraform.ParseResourceAddress(s) + if err != nil { + panic(err) + } + return addr +} diff --git a/command/show.go b/command/show.go index 254644863be8..d8d1c84efef7 100644 --- a/command/show.go +++ b/command/show.go @@ -110,11 +110,8 @@ func (c *ShowCommand) Run(args []string) int { } if plan != nil { - c.Ui.Output(format.Plan(&format.PlanOpts{ - Plan: plan, - Color: c.Colorize(), - ModuleDepth: moduleDepth, - })) + dispPlan := format.NewPlan(plan) + c.Ui.Output(dispPlan.Format(c.Colorize())) return 0 } diff --git a/terraform/diff.go b/terraform/diff.go index fd1687e7ed6c..9cbd5d88fa7a 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -23,6 +23,12 @@ const ( DiffUpdate DiffDestroy DiffDestroyCreate + + // DiffRefresh is only used in the UI for displaying diffs. + // Managed resource reads never appear in plan, and when data source + // reads appear they are represented as DiffCreate in core before + // transforming to DiffRefresh in the UI layer. + DiffRefresh // TODO: Actually use DiffRefresh in core too, for less confusion ) // multiVal matches the index key to a flatmapped set, list or map From d09b4a04da8e4dc07166718b8356c68cb35fbdf4 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 23 Aug 2017 17:11:57 -0700 Subject: [PATCH 3/7] command: show resource actions using resource addresses Previously we were using the internal resource id syntax in the UI. Now we'll use the standard user-facing resource address syntax instead. --- command/hook_ui.go | 25 ++++++++-------- terraform/resource.go | 40 ++++++++++++++++++++++++++ terraform/resource_test.go | 58 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 11 deletions(-) diff --git a/command/hook_ui.go b/command/hook_ui.go index 5d4690677cae..76b1ca59cff9 100644 --- a/command/hook_ui.go +++ b/command/hook_ui.go @@ -65,6 +65,7 @@ func (h *UiHook) PreApply( } id := n.HumanId() + addr := n.ResourceAddress() op := uiResourceModify if d.Destroy { @@ -142,7 +143,7 @@ func (h *UiHook) PreApply( h.ui.Output(h.Colorize.Color(fmt.Sprintf( "[reset][bold]%s: %s%s[reset]%s", - id, + addr, operation, stateIdSuffix, attrString))) @@ -210,6 +211,7 @@ func (h *UiHook) PostApply( applyerr error) (terraform.HookAction, error) { id := n.HumanId() + addr := n.ResourceAddress() h.l.Lock() state := h.resources[id] @@ -244,7 +246,7 @@ func (h *UiHook) PostApply( colorized := h.Colorize.Color(fmt.Sprintf( "[reset][bold]%s: %s after %s%s[reset]", - id, msg, time.Now().Round(time.Second).Sub(state.Start), stateIdSuffix)) + addr, msg, time.Now().Round(time.Second).Sub(state.Start), stateIdSuffix)) h.ui.Output(colorized) @@ -260,10 +262,10 @@ func (h *UiHook) PreDiff( func (h *UiHook) PreProvision( n *terraform.InstanceInfo, provId string) (terraform.HookAction, error) { - id := n.HumanId() + addr := n.ResourceAddress() h.ui.Output(h.Colorize.Color(fmt.Sprintf( "[reset][bold]%s: Provisioning with '%s'...[reset]", - id, provId))) + addr, provId))) return terraform.HookActionContinue, nil } @@ -271,11 +273,11 @@ func (h *UiHook) ProvisionOutput( n *terraform.InstanceInfo, provId string, msg string) { - id := n.HumanId() + addr := n.ResourceAddress() var buf bytes.Buffer buf.WriteString(h.Colorize.Color("[reset]")) - prefix := fmt.Sprintf("%s (%s): ", id, provId) + prefix := fmt.Sprintf("%s (%s): ", addr, provId) s := bufio.NewScanner(strings.NewReader(msg)) s.Split(scanLines) for s.Scan() { @@ -293,7 +295,7 @@ func (h *UiHook) PreRefresh( s *terraform.InstanceState) (terraform.HookAction, error) { h.once.Do(h.init) - id := n.HumanId() + addr := n.ResourceAddress() var stateIdSuffix string // Data resources refresh before they have ids, whereas managed @@ -304,7 +306,7 @@ func (h *UiHook) PreRefresh( h.ui.Output(h.Colorize.Color(fmt.Sprintf( "[reset][bold]%s: Refreshing state...%s", - id, stateIdSuffix))) + addr, stateIdSuffix))) return terraform.HookActionContinue, nil } @@ -313,9 +315,10 @@ func (h *UiHook) PreImportState( id string) (terraform.HookAction, error) { h.once.Do(h.init) + addr := n.ResourceAddress() h.ui.Output(h.Colorize.Color(fmt.Sprintf( "[reset][bold]%s: Importing from ID %q...", - n.HumanId(), id))) + addr, id))) return terraform.HookActionContinue, nil } @@ -324,9 +327,9 @@ func (h *UiHook) PostImportState( s []*terraform.InstanceState) (terraform.HookAction, error) { h.once.Do(h.init) - id := n.HumanId() + addr := n.ResourceAddress() h.ui.Output(h.Colorize.Color(fmt.Sprintf( - "[reset][bold][green]%s: Import complete!", id))) + "[reset][bold][green]%s: Import complete!", addr))) for _, s := range s { h.ui.Output(h.Colorize.Color(fmt.Sprintf( "[reset][green] Imported %s (ID: %s)", diff --git a/terraform/resource.go b/terraform/resource.go index 0acf0beb2a17..a8cd8dd9f00c 100644 --- a/terraform/resource.go +++ b/terraform/resource.go @@ -88,6 +88,46 @@ func (i *InstanceInfo) HumanId() string { i.Id) } +// ResourceAddress returns the address of the resource that the receiver is describing. +func (i *InstanceInfo) ResourceAddress() *ResourceAddress { + // GROSS: for tainted and deposed instances, their status gets appended + // to i.Id to create a unique id for the graph node. Historically these + // ids were displayed to the user, so it's designed to be human-readable: + // "aws_instance.bar.0 (deposed #0)" + // + // So here we detect such suffixes and try to interpret them back to + // their original meaning so we can then produce a ResourceAddress + // with a suitable InstanceType. + id := i.Id + instanceType := TypeInvalid + if idx := strings.Index(id, " ("); idx != -1 { + remain := id[idx:] + id = id[:idx] + + switch { + case strings.Contains(remain, "tainted"): + instanceType = TypeTainted + case strings.Contains(remain, "deposed"): + instanceType = TypeDeposed + } + } + + addr, err := parseResourceAddressInternal(id) + if err != nil { + // should never happen, since that would indicate a bug in the + // code that constructed this InstanceInfo. + panic(fmt.Errorf("InstanceInfo has invalid Id %s", id)) + } + if len(i.ModulePath) > 1 { + addr.Path = i.ModulePath[1:] // trim off "root" prefix, which is implied + } + if instanceType != TypeInvalid { + addr.InstanceTypeSet = true + addr.InstanceType = instanceType + } + return addr +} + func (i *InstanceInfo) uniqueId() string { prefix := i.HumanId() if v := i.uniqueExtra; v != "" { diff --git a/terraform/resource_test.go b/terraform/resource_test.go index 750e7060d18c..31d511e5c5cd 100644 --- a/terraform/resource_test.go +++ b/terraform/resource_test.go @@ -3,6 +3,7 @@ package terraform import ( "fmt" "reflect" + "strconv" "testing" "github.com/hashicorp/hil" @@ -46,6 +47,63 @@ func TestInstanceInfo(t *testing.T) { } } +func TestInstanceInfoResourceAddress(t *testing.T) { + tests := []struct { + Input *InstanceInfo + Want string + }{ + { + &InstanceInfo{ + Id: "test_resource.baz", + }, + "test_resource.baz", + }, + { + &InstanceInfo{ + Id: "test_resource.baz", + ModulePath: rootModulePath, + }, + "test_resource.baz", + }, + { + &InstanceInfo{ + Id: "test_resource.baz", + ModulePath: []string{"root", "foo"}, + }, + "module.foo.test_resource.baz", + }, + { + &InstanceInfo{ + Id: "test_resource.baz", + ModulePath: []string{"root", "foo", "bar"}, + }, + "module.foo.module.bar.test_resource.baz", + }, + { + &InstanceInfo{ + Id: "test_resource.baz (tainted)", + }, + "test_resource.baz.tainted", + }, + { + &InstanceInfo{ + Id: "test_resource.baz (deposed #0)", + }, + "test_resource.baz.deposed", + }, + } + + for i, test := range tests { + t.Run(strconv.Itoa(i), func(t *testing.T) { + gotAddr := test.Input.ResourceAddress() + got := gotAddr.String() + if got != test.Want { + t.Fatalf("wrong result\ngot: %s\nwant: %s", got, test.Want) + } + }) + } +} + func TestResourceConfigGet(t *testing.T) { cases := []struct { Config map[string]interface{} From 09a42195b1d88b188b2ce9913c26d1f5d6b34af2 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 23 Aug 2017 17:23:33 -0700 Subject: [PATCH 4/7] core: don't advertise data source destroy via hooks The fact that we clean up data source state by applying a "destroy" action for them is an implementation detail, and so should not be visible to outside callers or to the user. Signalling these as real destroys creates confusion for users because they see Terraform say things like: data.template_file.foo: Refreshing state..." ...which, to an understandably-nervous sysadmin, might make them suspect that the underlying object was deleted, rather than just Terraform's record of it. --- terraform/eval_apply.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 2f6a4973e454..36c98458eb96 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -112,7 +112,7 @@ func (n *EvalApplyPre) Eval(ctx EvalContext) (interface{}, error) { } state.init() - { + if resourceHasUserVisibleApply(n.Info) { // Call post-apply hook err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PreApply(n.Info, state, diff) @@ -136,7 +136,7 @@ type EvalApplyPost struct { func (n *EvalApplyPost) Eval(ctx EvalContext) (interface{}, error) { state := *n.State - { + if resourceHasUserVisibleApply(n.Info) { // Call post-apply hook err := ctx.Hook(func(h Hook) (HookAction, error) { return h.PostApply(n.Info, state, *n.Error) @@ -149,6 +149,22 @@ func (n *EvalApplyPost) Eval(ctx EvalContext) (interface{}, error) { return nil, *n.Error } +// resourceHasUserVisibleApply returns true if the given resource is one where +// apply actions should be exposed to the user. +// +// Certain resources do apply actions only as an implementation detail, so +// these should not be advertised to code outside of this package. +func resourceHasUserVisibleApply(info *InstanceInfo) bool { + addr := info.ResourceAddress() + + // Only managed resources have user-visible apply actions. + // In particular, this excludes data resources since we "apply" these + // only as an implementation detail of removing them from state when + // they are destroyed. (When reading, they don't get here at all because + // we present them as "Refresh" actions.) + return addr.Mode == config.ManagedResourceMode +} + // EvalApplyProvisioners is an EvalNode implementation that executes // the provisioners for a resource. // From 0dcdb94af94c30062809997e76392006b315fe84 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 31 Aug 2017 18:05:01 -0700 Subject: [PATCH 5/7] core: add testHook for testing correct interaction with hooks --- terraform/hook_test.go | 86 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/terraform/hook_test.go b/terraform/hook_test.go index a16923f42167..caa4f80878e2 100644 --- a/terraform/hook_test.go +++ b/terraform/hook_test.go @@ -7,3 +7,89 @@ import ( func TestNilHook_impl(t *testing.T) { var _ Hook = new(NilHook) } + +// testHook is a Hook implementation that logs the calls it receives. +// It is intended for testing that core code is emitting the correct hooks +// for a given situation. +type testHook struct { + Calls []*testHookCall +} + +// testHookCall represents a single call in testHook. +// This hook just logs string names to make it easy to write "want" expressions +// in tests that can DeepEqual against the real calls. +type testHookCall struct { + Action string + InstanceID string +} + +func (h *testHook) PreApply(i *InstanceInfo, s *InstanceState, d *InstanceDiff) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PreApply", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) PostApply(i *InstanceInfo, s *InstanceState, err error) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PostApply", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) PreDiff(i *InstanceInfo, s *InstanceState) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PreDiff", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) PostDiff(i *InstanceInfo, d *InstanceDiff) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PostDiff", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) PreProvisionResource(i *InstanceInfo, s *InstanceState) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PreProvisionResource", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) PostProvisionResource(i *InstanceInfo, s *InstanceState) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PostProvisionResource", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) PreProvision(i *InstanceInfo, n string) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PreProvision", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) PostProvision(i *InstanceInfo, n string, err error) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PostProvision", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) ProvisionOutput(i *InstanceInfo, n string, m string) { + h.Calls = append(h.Calls, &testHookCall{"ProvisionOutput", i.ResourceAddress().String()}) +} + +func (h *testHook) PreRefresh(i *InstanceInfo, s *InstanceState) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PreRefresh", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) PostRefresh(i *InstanceInfo, s *InstanceState) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PostRefresh", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) PreImportState(i *InstanceInfo, n string) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PreImportState", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) PostImportState(i *InstanceInfo, ss []*InstanceState) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PostImportState", i.ResourceAddress().String()}) + return HookActionContinue, nil +} + +func (h *testHook) PostStateUpdate(s *State) (HookAction, error) { + h.Calls = append(h.Calls, &testHookCall{"PostStateUpdate", ""}) + return HookActionContinue, nil +} + +var _ Hook = new(testHook) From f7125b3067da1123668ce59648eabbb297f784ff Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 31 Aug 2017 18:06:13 -0700 Subject: [PATCH 6/7] core: test that we skip hooks for data source destroy Data source destroy is an implementation detail and not something that external callers should see or expect. --- terraform/context_apply_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 48d8a9f490b8..fc45c6ac032e 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -1519,6 +1519,7 @@ func TestContext2Apply_destroyData(t *testing.T) { }, }, } + hook := &testHook{} ctx := testContext2(t, &ContextOpts{ Module: m, ProviderResolver: ResourceProviderResolverFixed( @@ -1528,6 +1529,7 @@ func TestContext2Apply_destroyData(t *testing.T) { ), State: state, Destroy: true, + Hooks: []Hook{hook}, }) if p, err := ctx.Plan(); err != nil { @@ -1548,6 +1550,15 @@ func TestContext2Apply_destroyData(t *testing.T) { if got := len(newState.Modules[0].Resources); got != 0 { t.Fatalf("state has %d resources after destroy; want 0", got) } + + wantHookCalls := []*testHookCall{ + {"PreDiff", "data.null_data_source.testing"}, + {"PostDiff", "data.null_data_source.testing"}, + {"PostStateUpdate", ""}, + } + if !reflect.DeepEqual(hook.Calls, wantHookCalls) { + t.Errorf("wrong hook calls\ngot: %swant: %s", spew.Sdump(hook.Calls), spew.Sdump(wantHookCalls)) + } } // https://github.com/hashicorp/terraform/pull/5096 From cbbcce828f608b91f5ce1d8a9e94408c43914bf8 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 31 Aug 2017 19:19:06 -0700 Subject: [PATCH 7/7] command: various adjustments to the diff presentation The previous diff presentation was rather "wordy", and not very friendly to those who can't see color either because they have color-blindness or because they don't have a color-supporting terminal. This new presentation uses the actual symbols used in the plan output and tries to be more concise. It also uses some framing characters to try to separate the different stages of "terraform plan" to make it easier to visually navigate. The apply command also adopts this new plan presentation, in preparation for "terraform apply" (with interactive plan confirmation) becoming the primary, safe workflow in the next major release. Finally, we standardize on the terminology "perform" and "actions" rather than "execute" and "changes" to reflect the fact that reading is now an action and that isn't actually a _change_. --- backend/local/backend_apply.go | 26 ++-------- backend/local/backend_plan.go | 92 ++++++++++++++++++++++------------ command/format/plan.go | 42 ++++++++++++---- 3 files changed, 98 insertions(+), 62 deletions(-) diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index 608c44411750..4463431815d3 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -108,19 +108,15 @@ func (b *Local) opApply( "There is no undo. Only 'yes' will be accepted to confirm." query = "Do you really want to destroy?" } else { - desc = "Terraform will apply the changes described above.\n" + + desc = "Terraform will perform the actions described above.\n" + "Only 'yes' will be accepted to approve." - query = "Do you want to apply these changes?" + query = "Do you want to perform these actions?" } if !trivialPlan { // Display the plan of what we are going to apply/destroy. - if op.Destroy { - op.UIOut.Output("\n" + strings.TrimSpace(approveDestroyPlanHeader) + "\n") - } else { - op.UIOut.Output("\n" + strings.TrimSpace(approvePlanHeader) + "\n") - } - op.UIOut.Output(dispPlan.Format(b.Colorize())) + b.renderPlan(dispPlan) + b.CLI.Output("") } v, err := op.UIIn.Input(&terraform.InputOpts{ @@ -337,17 +333,3 @@ Terraform encountered an error attempting to save the state before canceling the current operation. Once the operation is complete another attempt will be made to save the final state. ` - -const approvePlanHeader = ` -The Terraform execution plan has been generated and is shown below. -Resources are shown in alphabetical order for quick scanning. Green resources -will be created (or destroyed and then created if an existing resource -exists), yellow resources are being changed in-place, and red resources -will be destroyed. Cyan entries are data sources to be read. -` - -const approveDestroyPlanHeader = ` -The Terraform destroy plan has been generated and is shown below. -Resources are shown in alphabetical order for quick scanning. -Resources shown in red will be destroyed. -` diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go index 83d1733dec76..905f89f1a917 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -1,6 +1,7 @@ package local import ( + "bytes" "context" "fmt" "log" @@ -95,6 +96,9 @@ func (b *Local) opPlan( runningOp.Err = errwrap.Wrapf("Error refreshing state: {{err}}", err) return } + if b.CLI != nil { + b.CLI.Output("\n------------------------------------------------------------------------") + } } // Perform the plan @@ -135,27 +139,60 @@ func (b *Local) opPlan( if b.CLI != nil { dispPlan := format.NewPlan(plan) if dispPlan.Empty() { - b.CLI.Output(b.Colorize().Color(strings.TrimSpace(planNoChanges))) + b.CLI.Output("\n" + b.Colorize().Color(strings.TrimSpace(planNoChanges))) return } + b.renderPlan(dispPlan) + + b.CLI.Output("\n------------------------------------------------------------------------") + if path := op.PlanOutPath; path == "" { - b.CLI.Output(strings.TrimSpace(planHeaderNoOutput) + "\n") + b.CLI.Output(fmt.Sprintf( + "\n" + strings.TrimSpace(planHeaderNoOutput) + "\n", + )) } else { b.CLI.Output(fmt.Sprintf( - strings.TrimSpace(planHeaderYesOutput)+"\n", - path)) + "\n"+strings.TrimSpace(planHeaderYesOutput)+"\n", + path, path, + )) } + } +} - b.CLI.Output(dispPlan.Format(b.Colorize())) +func (b *Local) renderPlan(dispPlan *format.Plan) { - stats := dispPlan.Stats() - b.CLI.Output(b.Colorize().Color(fmt.Sprintf( - "[reset][bold]Plan:[reset] "+ - "%d to add, %d to change, %d to destroy.", - stats.ToAdd, stats.ToChange, stats.ToDestroy, - ))) + headerBuf := &bytes.Buffer{} + fmt.Fprintf(headerBuf, "\n%s\n", strings.TrimSpace(planHeaderIntro)) + counts := dispPlan.ActionCounts() + if counts[terraform.DiffCreate] > 0 { + fmt.Fprintf(headerBuf, "%s create\n", format.DiffActionSymbol(terraform.DiffCreate)) + } + if counts[terraform.DiffUpdate] > 0 { + fmt.Fprintf(headerBuf, "%s update in-place\n", format.DiffActionSymbol(terraform.DiffUpdate)) + } + if counts[terraform.DiffDestroy] > 0 { + fmt.Fprintf(headerBuf, "%s destroy\n", format.DiffActionSymbol(terraform.DiffDestroy)) } + if counts[terraform.DiffDestroyCreate] > 0 { + fmt.Fprintf(headerBuf, "%s destroy and then create replacement\n", format.DiffActionSymbol(terraform.DiffDestroyCreate)) + } + if counts[terraform.DiffRefresh] > 0 { + fmt.Fprintf(headerBuf, "%s read (data resources)\n", format.DiffActionSymbol(terraform.DiffRefresh)) + } + + b.CLI.Output(b.Colorize().Color(headerBuf.String())) + + b.CLI.Output("Terraform will perform the following actions:\n") + + b.CLI.Output(dispPlan.Format(b.Colorize())) + + stats := dispPlan.Stats() + b.CLI.Output(b.Colorize().Color(fmt.Sprintf( + "[reset][bold]Plan:[reset] "+ + "%d to add, %d to change, %d to destroy.", + stats.ToAdd, stats.ToChange, stats.ToDestroy, + ))) } const planErrNoConfig = ` @@ -168,37 +205,30 @@ flag or create a single empty configuration file. Otherwise, please create a Terraform configuration file in the path being executed and try again. ` +const planHeaderIntro = ` +An execution plan has been generated and is shown below. +Resource actions are indicated with the following symbols: +` + const planHeaderNoOutput = ` -The Terraform execution plan has been generated and is shown below. -Resources are shown in alphabetical order for quick scanning. Green resources -will be created (or destroyed and then created if an existing resource -exists), yellow resources are being changed in-place, and red resources -will be destroyed. Cyan entries are data sources to be read. - -Note: You didn't specify an "-out" parameter to save this plan, so when -"apply" is called, Terraform can't guarantee this is what will execute. +Note: You didn't specify an "-out" parameter to save this plan, so Terraform +can't guarantee that exactly these actions will be performed if +"terraform apply" is subsequently run. ` const planHeaderYesOutput = ` -The Terraform execution plan has been generated and is shown below. -Resources are shown in alphabetical order for quick scanning. Green resources -will be created (or destroyed and then created if an existing resource -exists), yellow resources are being changed in-place, and red resources -will be destroyed. Cyan entries are data sources to be read. - -Your plan was also saved to the path below. Call the "apply" subcommand -with this plan file and Terraform will exactly execute this execution -plan. +This plan was saved to: %s -Path: %s +To perform exactly these actions, run the following command to apply: + terraform apply %q ` const planNoChanges = ` [reset][bold][green]No changes. Infrastructure is up-to-date.[reset][green] This means that Terraform did not detect any differences between your -configuration and real physical resources that exist. As a result, Terraform -doesn't need to do anything. +configuration and real physical resources that exist. As a result, no +actions need to be performed. ` const planRefreshing = ` diff --git a/command/format/plan.go b/command/format/plan.go index 1220b85a5e7d..728345e0a8b9 100644 --- a/command/format/plan.go +++ b/command/format/plan.go @@ -221,11 +221,39 @@ func (p *Plan) Stats() PlanStats { return ret } +// ActionCounts returns the number of diffs for each action type +func (p *Plan) ActionCounts() map[terraform.DiffChangeType]int { + ret := map[terraform.DiffChangeType]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 terraform.DiffChangeType) string { + switch action { + case terraform.DiffDestroyCreate: + return "[red]-[reset]/[green]+[reset]" + case terraform.DiffCreate: + return " [green]+[reset]" + case terraform.DiffDestroy: + return " [red]-[reset]" + case terraform.DiffRefresh: + return " [cyan]<=[reset]" + default: + return " [yellow]~[reset]" + } +} + // 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) { @@ -235,31 +263,27 @@ func formatPlanInstanceDiff(buf *bytes.Buffer, r *InstanceDiff, keyLen int, colo // for change, red for delete), and symbol, and output the // resource header. color := "yellow" - symbol := " ~" + symbol := DiffActionSymbol(r.Action) oldValues := true switch r.Action { case terraform.DiffDestroyCreate: color = "yellow" - symbol = "[red]-[reset]/[green]+[reset][yellow]" case terraform.DiffCreate: color = "green" - symbol = " +" oldValues = false case terraform.DiffDestroy: color = "red" - symbol = " -" case terraform.DiffRefresh: - symbol = " <=" color = "cyan" oldValues = false } var extraStr string if r.Tainted { - extraStr = extraStr + colorizer.Color(" (tainted)") + extraStr = extraStr + " (tainted)" } if r.Deposed { - extraStr = extraStr + colorizer.Color(" (deposed)") + extraStr = extraStr + " (deposed)" } if r.Action == terraform.DiffDestroyCreate { extraStr = extraStr + colorizer.Color(" [red][bold](new resource required)") @@ -267,8 +291,8 @@ func formatPlanInstanceDiff(buf *bytes.Buffer, r *InstanceDiff, keyLen int, colo buf.WriteString( colorizer.Color(fmt.Sprintf( - "[%s]%s %s%s\n", - color, symbol, addrStr, extraStr, + "[%s]%s [%s]%s%s\n", + color, symbol, color, addrStr, extraStr, )), )