From 275a44f5522a06a23f784130ce543d0f8b5e29c2 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Sat, 13 Oct 2018 10:47:46 -0700 Subject: [PATCH] command: Reinstate object ids in the UIHook progress logs We used to treat the "id" attribute of a resource as special and elevate it into its own struct field "ID" in the state, but the new state format and provider protocol treats it just as any other attribute. However, it's still useful to show the value of a single identifying attribute when there isn't room in the UI for showing all of the attributes, and so here we take a new strategy of considering "id" along with some other conventional names as special only in the UI layer. This new heuristic approach can be adjusted over time as new provider patterns emerge, but for now it covers some common conventions we've seen in real providers. With that said, since all existing providers made for Terraform versions prior to v0.12 were forced to set "id", we won't see any use of other attributes here until providers are updated to remove the placeholder ids they were generating in cases where an id was not actually relevant but was forced by the old protocol. At that point the UX should be improved by showing a more relevant attribute instead. We now also allow for the possibility of no id at all, since that is valid for resources that exist only within the Terraform state, like the ones from the "random" and "tls" providers. --- command/format/object_id.go | 123 ++++++++++++++++++++ command/format/object_id_test.go | 194 +++++++++++++++++++++++++++++++ command/hook_ui.go | 48 +++++--- command/hook_ui_test.go | 30 ++--- 4 files changed, 362 insertions(+), 33 deletions(-) create mode 100644 command/format/object_id.go create mode 100644 command/format/object_id_test.go diff --git a/command/format/object_id.go b/command/format/object_id.go new file mode 100644 index 000000000..85ebbfec5 --- /dev/null +++ b/command/format/object_id.go @@ -0,0 +1,123 @@ +package format + +import ( + "github.com/zclconf/go-cty/cty" +) + +// ObjectValueID takes a value that is assumed to be an object representation +// of some resource instance object and attempts to heuristically find an +// attribute of it that is likely to be a unique identifier in the remote +// system that it belongs to which will be useful to the user. +// +// If such an attribute is found, its name and string value intended for +// display are returned. Both returned strings are empty if no such attribute +// exists, in which case the caller should assume that the resource instance +// address within the Terraform configuration is the best available identifier. +// +// This is only a best-effort sort of thing, relying on naming conventions in +// our resource type schemas. The result is not guaranteed to be unique, but +// should generally be suitable for display to an end-user anyway. +// +// This function will panic if the given value is not of an object type. +func ObjectValueID(obj cty.Value) (k, v string) { + if obj.IsNull() || !obj.IsKnown() { + return "", "" + } + + atys := obj.Type().AttributeTypes() + + switch { + + case atys["id"] == cty.String: + v := obj.GetAttr("id") + if v.IsKnown() && !v.IsNull() { + return "id", v.AsString() + } + + case atys["name"] == cty.String: + // "name" isn't always globally unique, but if there isn't also an + // "id" then it _often_ is, in practice. + v := obj.GetAttr("name") + if v.IsKnown() && !v.IsNull() { + return "name", v.AsString() + } + } + + return "", "" +} + +// ObjectValueName takes a value that is assumed to be an object representation +// of some resource instance object and attempts to heuristically find an +// attribute of it that is likely to be a human-friendly name in the remote +// system that it belongs to which will be useful to the user. +// +// If such an attribute is found, its name and string value intended for +// display are returned. Both returned strings are empty if no such attribute +// exists, in which case the caller should assume that the resource instance +// address within the Terraform configuration is the best available identifier. +// +// This is only a best-effort sort of thing, relying on naming conventions in +// our resource type schemas. The result is not guaranteed to be unique, but +// should generally be suitable for display to an end-user anyway. +// +// Callers that use both ObjectValueName and ObjectValueID at the same time +// should be prepared to get the same attribute key and value from both in +// some cases, since there is overlap betweek the id-extraction and +// name-extraction heuristics. +// +// This function will panic if the given value is not of an object type. +func ObjectValueName(obj cty.Value) (k, v string) { + if obj.IsNull() || !obj.IsKnown() { + return "", "" + } + + atys := obj.Type().AttributeTypes() + + switch { + + case atys["name"] == cty.String: + v := obj.GetAttr("name") + if v.IsKnown() && !v.IsNull() { + return "name", v.AsString() + } + + case atys["tags"].IsMapType() && atys["tags"].ElementType() == cty.String: + tags := obj.GetAttr("tags") + if tags.IsNull() || !tags.IsWhollyKnown() { + break + } + + switch { + case tags.HasIndex(cty.StringVal("name")).RawEquals(cty.True): + v := tags.Index(cty.StringVal("name")) + if v.IsKnown() && !v.IsNull() { + return "tags.name", v.AsString() + } + case tags.HasIndex(cty.StringVal("Name")).RawEquals(cty.True): + // AWS-style naming convention + v := tags.Index(cty.StringVal("Name")) + if v.IsKnown() && !v.IsNull() { + return "tags.Name", v.AsString() + } + } + } + + return "", "" +} + +// ObjectValueIDOrName is a convenience wrapper around both ObjectValueID +// and ObjectValueName (in that preference order) to try to extract some sort +// of human-friendly descriptive string value for an object as additional +// context about an object when it is being displayed in a compact way (where +// not all of the attributes are visible.) +// +// Just as with the two functions it wraps, it is a best-effort and may return +// two empty strings if no suitable attribute can be found for a given object. +func ObjectValueIDOrName(obj cty.Value) (k, v string) { + k, v = ObjectValueID(obj) + if k != "" { + return + } + k, v = ObjectValueName(obj) + return +} diff --git a/command/format/object_id_test.go b/command/format/object_id_test.go new file mode 100644 index 000000000..b562234fe --- /dev/null +++ b/command/format/object_id_test.go @@ -0,0 +1,194 @@ +package format + +import ( + "fmt" + "testing" + + "github.com/zclconf/go-cty/cty" +) + +func TestObjectValueIDOrName(t *testing.T) { + tests := []struct{ + obj cty.Value + id [2]string + name [2]string + either [2]string + }{ + { + cty.NullVal(cty.EmptyObject), + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + { + cty.UnknownVal(cty.EmptyObject), + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + { + cty.EmptyObjectVal, + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("foo-123"), + }), + [...]string{"id", "foo-123"}, + [...]string{"", ""}, + [...]string{"id", "foo-123"}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("foo-123"), + "name": cty.StringVal("awesome-foo"), + }), + [...]string{"id", "foo-123"}, + [...]string{"name", "awesome-foo"}, + [...]string{"id", "foo-123"}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "name": cty.StringVal("awesome-foo"), + }), + [...]string{"name", "awesome-foo"}, + [...]string{"name", "awesome-foo"}, + [...]string{"name", "awesome-foo"}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "name": cty.StringVal("awesome-foo"), + "tags": cty.MapVal(map[string]cty.Value{ + "Name": cty.StringVal("My Awesome Foo"), + }), + }), + [...]string{"name", "awesome-foo"}, + [...]string{"name", "awesome-foo"}, + [...]string{"name", "awesome-foo"}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "tags": cty.MapVal(map[string]cty.Value{ + "Name": cty.StringVal("My Awesome Foo"), + "name": cty.StringVal("awesome-foo"), + }), + }), + [...]string{"", ""}, + [...]string{"tags.name", "awesome-foo"}, + [...]string{"tags.name", "awesome-foo"}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "tags": cty.MapVal(map[string]cty.Value{ + "Name": cty.StringVal("My Awesome Foo"), + }), + }), + [...]string{"", ""}, + [...]string{"tags.Name", "My Awesome Foo"}, + [...]string{"tags.Name", "My Awesome Foo"}, + }, + + // The following are degenerate cases, included to make sure we don't + // crash when we encounter them. If you're here fixing a reported panic + // in this formatter, this is the place to add a new test case. + { + cty.ObjectVal(map[string]cty.Value{ + "id": cty.True, + }), + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "id": cty.NullVal(cty.String), + }), + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + }), + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "tags": cty.StringVal("foo"), + }), + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "tags": cty.NullVal(cty.Map(cty.String)), + }), + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "tags": cty.UnknownVal(cty.Map(cty.String)), + }), + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "tags": cty.MapVal(map[string]cty.Value{ + "Name": cty.True, + }), + }), + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "tags": cty.MapVal(map[string]cty.Value{ + "Name": cty.UnknownVal(cty.String), + }), + }), + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + { + cty.ObjectVal(map[string]cty.Value{ + "tags": cty.MapVal(map[string]cty.Value{ + "Name": cty.NullVal(cty.String), + }), + }), + [...]string{"", ""}, + [...]string{"", ""}, + [...]string{"", ""}, + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("%#v", test.obj), func (t *testing.T) { + obj := test.obj + gotIDKey, gotIDVal := ObjectValueID(obj) + gotNameKey, gotNameVal := ObjectValueName(obj) + gotEitherKey, gotEitherVal := ObjectValueIDOrName(obj) + + if got, want := [...]string{gotIDKey, gotIDVal}, test.id; got != want { + t.Errorf("wrong ObjectValueID result\ngot: %#v\nwant: %#v", got, want) + } + if got, want := [...]string{gotNameKey, gotNameVal}, test.name; got != want { + t.Errorf("wrong ObjectValueName result\ngot: %#v\nwant: %#v", got, want) + } + if got, want := [...]string{gotEitherKey, gotEitherVal}, test.either; got != want { + t.Errorf("wrong ObjectValueIDOrName result\ngot: %#v\nwant: %#v", got, want) + } + }) + } +} diff --git a/command/hook_ui.go b/command/hook_ui.go index e90573042..f105930d7 100644 --- a/command/hook_ui.go +++ b/command/hook_ui.go @@ -15,6 +15,7 @@ import ( "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/command/format" "github.com/hashicorp/terraform/plans" "github.com/hashicorp/terraform/providers" "github.com/hashicorp/terraform/states" @@ -41,10 +42,10 @@ var _ terraform.Hook = (*UiHook)(nil) // uiResourceState tracks the state of a single resource type uiResourceState struct { - Name string - ResourceId string - Op uiResourceOp - Start time.Time + DispAddr string + IDKey, IDValue string + Op uiResourceOp + Start time.Time DoneCh chan struct{} // To be used for cancellation @@ -71,6 +72,7 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, var operation string var op uiResourceOp + idKey, idValue := format.ObjectValueIDOrName(priorState) switch action { case plans.Delete: operation = "Destroying..." @@ -101,11 +103,6 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, dAttrs := map[string]terraform.ResourceAttrDiff{} keys := make([]string, 0, len(dAttrs)) for key, _ := range dAttrs { - // Skip the ID since we do that specially - if key == "id" { - continue - } - keys = append(keys, key) if len(key) > keyLen { keyLen = len(key) @@ -141,7 +138,15 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, attrString = "\n " + attrString } - var stateId, stateIdSuffix string + var stateIdSuffix string + if idKey != "" && idValue != "" { + stateIdSuffix = fmt.Sprintf(" [%s=%s]", idKey, idValue) + } else { + // Make sure they are both empty so we can deal with this more + // easily in the other hook methods. + idKey = "" + idValue = "" + } h.ui.Output(h.Colorize.Color(fmt.Sprintf( "[reset][bold]%s: %s%s[reset]%s", @@ -151,10 +156,11 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, attrString, ))) - id := addr.String() + key := addr.String() uiState := uiResourceState{ - Name: id, - ResourceId: stateId, + DispAddr: key, + IDKey: idKey, + IDValue: idValue, Op: op, Start: time.Now().Round(time.Second), DoneCh: make(chan struct{}), @@ -162,7 +168,7 @@ func (h *UiHook) PreApply(addr addrs.AbsResourceInstance, gen states.Generation, } h.l.Lock() - h.resources[id] = uiState + h.resources[key] = uiState h.l.Unlock() // Start goroutine that shows progress @@ -195,13 +201,13 @@ func (h *UiHook) stillApplying(state uiResourceState) { } idSuffix := "" - if v := state.ResourceId; v != "" { - idSuffix = fmt.Sprintf("ID: %s, ", truncateId(v, maxIdLen)) + if state.IDKey != "" { + idSuffix = fmt.Sprintf("%s=%s, ", state.IDKey, truncateId(state.IDValue, maxIdLen)) } h.ui.Output(h.Colorize.Color(fmt.Sprintf( - "[reset][bold]%s: %s (%s%s elapsed)[reset]", - state.Name, + "[reset][bold]%s: %s [%s%s elapsed][reset]", + state.DispAddr, msg, idSuffix, time.Now().Round(time.Second).Sub(state.Start), @@ -223,6 +229,9 @@ func (h *UiHook) PostApply(addr addrs.AbsResourceInstance, gen states.Generation h.l.Unlock() var stateIdSuffix string + if k, v := format.ObjectValueID(newState); k != "" && v != "" { + stateIdSuffix = fmt.Sprintf(" [%s=%s]", k, v) + } var msg string switch state.Op { @@ -283,6 +292,9 @@ func (h *UiHook) PreRefresh(addr addrs.AbsResourceInstance, gen states.Generatio h.once.Do(h.init) var stateIdSuffix string + if k, v := format.ObjectValueID(priorState); k != "" && v != "" { + stateIdSuffix = fmt.Sprintf(" [%s=%s]", k, v) + } h.ui.Output(h.Colorize.Color(fmt.Sprintf( "[reset][bold]%s: Refreshing state...%s", diff --git a/command/hook_ui_test.go b/command/hook_ui_test.go index cce88d03c..0a871cf6a 100644 --- a/command/hook_ui_test.go +++ b/command/hook_ui_test.go @@ -41,11 +41,7 @@ func TestUiHookPreApply_periodicTimer(t *testing.T) { Name: "available", }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance) - priorState := cty.NullVal(cty.Object(map[string]cty.Type{ - "id": cty.String, - "names": cty.List(cty.String), - })) - plannedNewState := cty.ObjectVal(map[string]cty.Value{ + priorState := cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("2017-03-05 10:56:59.298784526 +0000 UTC"), "names": cty.ListVal([]cty.Value{ cty.StringVal("us-east-1a"), @@ -54,6 +50,10 @@ func TestUiHookPreApply_periodicTimer(t *testing.T) { cty.StringVal("us-east-1d"), }), }) + plannedNewState := cty.NullVal(cty.Object(map[string]cty.Type{ + "id": cty.String, + "names": cty.List(cty.String), + })) action, err := h.PreApply(addr, states.CurrentGen, plans.Delete, priorState, plannedNewState) if err != nil { @@ -70,10 +70,10 @@ func TestUiHookPreApply_periodicTimer(t *testing.T) { close(uiState.DoneCh) <-uiState.done - expectedOutput := `data.aws_availability_zones.available: Destroying... (ID: 2017-03-05 10:56:59.298784526 +0000 UTC) -data.aws_availability_zones.available: Still destroying... (ID: 2017-03-05 10:56:59.298784526 +0000 UTC, 1s elapsed) -data.aws_availability_zones.available: Still destroying... (ID: 2017-03-05 10:56:59.298784526 +0000 UTC, 2s elapsed) -data.aws_availability_zones.available: Still destroying... (ID: 2017-03-05 10:56:59.298784526 +0000 UTC, 3s elapsed) + expectedOutput := `data.aws_availability_zones.available: Destroying... [id=2017-03-05 10:56:59.298784526 +0000 UTC] +data.aws_availability_zones.available: Still destroying... [id=2017-03-05 10:56:59.298784526 +0000 UTC, 1s elapsed] +data.aws_availability_zones.available: Still destroying... [id=2017-03-05 10:56:59.298784526 +0000 UTC, 2s elapsed] +data.aws_availability_zones.available: Still destroying... [id=2017-03-05 10:56:59.298784526 +0000 UTC, 3s elapsed] ` output := ui.OutputWriter.String() if output != expectedOutput { @@ -111,11 +111,7 @@ func TestUiHookPreApply_destroy(t *testing.T) { Name: "available", }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance) - priorState := cty.NullVal(cty.Object(map[string]cty.Type{ - "id": cty.String, - "names": cty.List(cty.String), - })) - plannedNewState := cty.ObjectVal(map[string]cty.Value{ + priorState := cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("2017-03-05 10:56:59.298784526 +0000 UTC"), "names": cty.ListVal([]cty.Value{ cty.StringVal("us-east-1a"), @@ -124,6 +120,10 @@ func TestUiHookPreApply_destroy(t *testing.T) { cty.StringVal("us-east-1d"), }), }) + plannedNewState := cty.NullVal(cty.Object(map[string]cty.Type{ + "id": cty.String, + "names": cty.List(cty.String), + })) action, err := h.PreApply(addr, states.CurrentGen, plans.Delete, priorState, plannedNewState) if err != nil { @@ -138,7 +138,7 @@ func TestUiHookPreApply_destroy(t *testing.T) { close(uiState.DoneCh) <-uiState.done - expectedOutput := "data.aws_availability_zones.available: Destroying... (ID: 2017-03-05 10:56:59.298784526 +0000 UTC)\n" + expectedOutput := "data.aws_availability_zones.available: Destroying... [id=2017-03-05 10:56:59.298784526 +0000 UTC]\n" output := ui.OutputWriter.String() if output != expectedOutput { t.Fatalf("Output didn't match.\nExpected: %q\nGiven: %q", expectedOutput, output)