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.
This commit is contained in:
Martin Atkins 2018-10-13 10:47:46 -07:00
parent 43d5206d82
commit 275a44f552
4 changed files with 362 additions and 33 deletions

123
command/format/object_id.go Normal file
View File

@ -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
}

View File

@ -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)
}
})
}
}

View File

@ -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",

View File

@ -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)