Merge pull request #26189 from hashicorp/alisdair/console-formatter

repl: Improved value renderer for console outputs

command/apply.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/backend"
-	"github.com/hashicorp/terraform/configs/hcl2shim"
 	"github.com/hashicorp/terraform/repl"
 	"github.com/hashicorp/terraform/states"
 	"github.com/hashicorp/terraform/tfdiags"
@@ -345,17 +344,7 @@ func outputsAsString(state *states.State, modPath addrs.ModuleInstance, includeH
 				continue
 			}
 
-			// Our formatter still wants an old-style raw interface{} value, so
-			// for now we'll just shim it.
-			// FIXME: Port the formatter to work with cty.Value directly.
-			legacyVal := hcl2shim.ConfigValueFromHCL2(v.Value)
-			result, err := repl.FormatResult(legacyVal)
-			if err != nil {
-				// We can't really return errors from here, so we'll just have
-				// to stub this out. This shouldn't happen in practice anyway.
-				result = "<error during formatting>"
-			}
-
+			result := repl.FormatValue(v.Value, 0)
 			outputBuf.WriteString(fmt.Sprintf("%s = %s\n", k, result))
 		}
 	}

command/apply_test.go

@@ -1107,7 +1107,7 @@ func TestApply_sensitiveOutput(t *testing.T) {
 	}
 
 	output := ui.OutputWriter.String()
-	if !strings.Contains(output, "notsensitive = Hello world") {
+	if !strings.Contains(output, "notsensitive = \"Hello world\"") {
 		t.Fatalf("bad: output should contain 'notsensitive' output\n%s", output)
 	}
 	if !strings.Contains(output, "sensitive = <sensitive>") {

command/console_test.go

@@ -27,7 +27,7 @@ func TestConsole_basic(t *testing.T) {
 	defer testFixCwd(t, tmp, cwd)
 
 	p := testProvider()
-	ui := new(cli.MockUi)
+	ui := cli.NewMockUi()
 	c := &ConsoleCommand{
 		Meta: Meta{
 			testingOverrides: metaOverridesForProvider(p),
@@ -73,7 +73,7 @@ func TestConsole_tfvars(t *testing.T) {
 		},
 	}
 
-	ui := new(cli.MockUi)
+	ui := cli.NewMockUi()
 	c := &ConsoleCommand{
 		Meta: Meta{
 			testingOverrides: metaOverridesForProvider(p),
@@ -95,7 +95,7 @@ func TestConsole_tfvars(t *testing.T) {
 	}
 
 	actual := output.String()
-	if actual != "bar\n" {
+	if actual != "\"bar\"\n" {
 		t.Fatalf("bad: %q", actual)
 	}
 }
@@ -120,7 +120,7 @@ func TestConsole_unsetRequiredVars(t *testing.T) {
 			},
 		},
 	}
-	ui := new(cli.MockUi)
+	ui := cli.NewMockUi()
 	c := &ConsoleCommand{
 		Meta: Meta{
 			testingOverrides: metaOverridesForProvider(p),
@@ -140,21 +140,12 @@ func TestConsole_unsetRequiredVars(t *testing.T) {
 	code := c.Run(args)
 	outCloser()
 
-	// Because we're running "terraform console" in piped input mode, we're
-	// expecting it to return a nonzero exit status here but the message
-	// must be the one indicating that it did attempt to evaluate var.foo and
-	// got an unknown value in return, rather than an error about var.foo
-	// not being set or a failure to prompt for it.
-	if code == 0 {
-		t.Fatalf("unexpected success\n%s", ui.OutputWriter.String())
+	if code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
 	}
 
-	// The error message should be the one console produces when it encounters
-	// an unknown value.
-	got := ui.ErrorWriter.String()
-	want := `Error: Result depends on values that cannot be determined`
-	if !strings.Contains(got, want) {
-		t.Fatalf("wrong output\ngot:\n%s\n\nwant string containing %q", got, want)
+	if got, want := output.String(), "(known after apply)\n"; got != want {
+		t.Fatalf("unexpected output\n got: %q\nwant: %q", got, want)
 	}
 }
 
@@ -165,7 +156,7 @@ func TestConsole_modules(t *testing.T) {
 	defer testChdir(t, td)()
 
 	p := applyFixtureProvider()
-	ui := new(cli.MockUi)
+	ui := cli.NewMockUi()
 
 	c := &ConsoleCommand{
 		Meta: Meta{
@@ -175,8 +166,8 @@ func TestConsole_modules(t *testing.T) {
 	}
 
 	commands := map[string]string{
-		"module.child.myoutput\n":          "bar\n",
-		"module.count_child[0].myoutput\n": "bar\n",
+		"module.child.myoutput\n":          "\"bar\"\n",
+		"module.count_child[0].myoutput\n": "\"bar\"\n",
 		"local.foo\n":                      "3\n",
 	}
 

command/output.go

@@ -10,7 +10,6 @@ import (
 	ctyjson "github.com/zclconf/go-cty/cty/json"
 
 	"github.com/hashicorp/terraform/addrs"
-	"github.com/hashicorp/terraform/configs/hcl2shim"
 	"github.com/hashicorp/terraform/repl"
 	"github.com/hashicorp/terraform/states"
 	"github.com/hashicorp/terraform/tfdiags"
@@ -195,16 +194,7 @@ func (c *OutputCommand) Run(args []string) int {
 
 		c.Ui.Output(string(jsonOutput))
 	} else {
-		// Our formatter still wants an old-style raw interface{} value, so
-		// for now we'll just shim it.
-		// FIXME: Port the formatter to work with cty.Value directly.
-		legacyVal := hcl2shim.ConfigValueFromHCL2(v)
-		result, err := repl.FormatResult(legacyVal)
-		if err != nil {
-			diags = diags.Append(err)
-			c.showDiagnostics(diags)
-			return 1
-		}
+		result := repl.FormatValue(v, 0)
 		c.Ui.Output(result)
 	}
 

command/output_test.go

@@ -41,7 +41,7 @@ func TestOutput(t *testing.T) {
 	}
 
 	actual := strings.TrimSpace(ui.OutputWriter.String())
-	if actual != "bar" {
+	if actual != `"bar"` {
 		t.Fatalf("bad: %#v", actual)
 	}
 }
@@ -80,9 +80,19 @@ func TestOutput_nestedListAndMap(t *testing.T) {
 	}
 
 	actual := strings.TrimSpace(ui.OutputWriter.String())
-	expected := "foo = [\n  {\n    \"key\" = \"value\"\n    \"key2\" = \"value2\"\n  },\n  {\n    \"key\" = \"value\"\n  },\n]"
+	expected := strings.TrimSpace(`
+foo = tolist([
+  tomap({
+    "key" = "value"
+    "key2" = "value2"
+  }),
+  tomap({
+    "key" = "value"
+  }),
+])
+`)
 	if actual != expected {
-		t.Fatalf("wrong output\ngot:  %#v\nwant: %#v", actual, expected)
+		t.Fatalf("wrong output\ngot:  %s\nwant: %s", actual, expected)
 	}
 }
 
@@ -260,7 +270,7 @@ func TestOutput_blank(t *testing.T) {
 		t.Fatalf("bad: \n%s", ui.ErrorWriter.String())
 	}
 
-	expectedOutput := "foo = bar\nname = john-doe\n"
+	expectedOutput := "foo = \"bar\"\nname = \"john-doe\"\n"
 	output := ui.OutputWriter.String()
 	if output != expectedOutput {
 		t.Fatalf("wrong output\ngot:  %#v\nwant: %#v", output, expectedOutput)
@@ -393,7 +403,7 @@ func TestOutput_stateDefault(t *testing.T) {
 	}
 
 	actual := strings.TrimSpace(ui.OutputWriter.String())
-	if actual != "bar" {
+	if actual != `"bar"` {
 		t.Fatalf("bad: %#v", actual)
 	}
 }

repl/format.go

@@ -1,116 +1,118 @@
 package repl
 
 import (
-	"bufio"
-	"bytes"
 	"fmt"
-	"sort"
 	"strconv"
 	"strings"
+
+	"github.com/zclconf/go-cty/cty"
 )
 
-// FormatResult formats the given result value for human-readable output.
-//
-// The value must currently be a string, list, map, and any nested values
-// with those same types.
-func FormatResult(value interface{}) (string, error) {
-	return formatResult(value, false)
-}
-
-func formatResult(value interface{}, nested bool) (string, error) {
-	if value == nil {
-		return "null", nil
+// FormatValue formats a value in a way that resembles Terraform language syntax
+// and uses the type conversion functions where necessary to indicate exactly
+// what type it is given, so that equality test failures can be quickly
+// understood.
+func FormatValue(v cty.Value, indent int) string {
+	if !v.IsKnown() {
+		return "(known after apply)"
 	}
-	switch output := value.(type) {
-	case string:
-		if nested {
-			return fmt.Sprintf("%q", output), nil
-		}
-		return output, nil
-	case int:
-		return strconv.Itoa(output), nil
-	case float64:
-		return fmt.Sprintf("%g", output), nil
-	case bool:
+	if v.IsNull() {
+		ty := v.Type()
 		switch {
-		case output == true:
-			return "true", nil
+		case ty == cty.DynamicPseudoType:
+			return "null"
+		case ty == cty.String:
+			return "tostring(null)"
+		case ty == cty.Number:
+			return "tonumber(null)"
+		case ty == cty.Bool:
+			return "tobool(null)"
+		case ty.IsListType():
+			return fmt.Sprintf("tolist(null) /* of %s */", ty.ElementType().FriendlyName())
+		case ty.IsSetType():
+			return fmt.Sprintf("toset(null) /* of %s */", ty.ElementType().FriendlyName())
+		case ty.IsMapType():
+			return fmt.Sprintf("tomap(null) /* of %s */", ty.ElementType().FriendlyName())
 		default:
-			return "false", nil
+			return fmt.Sprintf("null /* %s */", ty.FriendlyName())
 		}
-	case []interface{}:
-		return formatListResult(output)
-	case map[string]interface{}:
-		return formatMapResult(output)
-	default:
-		return "", fmt.Errorf("unknown value type: %T", value)
 	}
+
+	ty := v.Type()
+	switch {
+	case ty.IsPrimitiveType():
+		switch ty {
+		case cty.String:
+			// FIXME: If it's a multi-line string, better to render it using
+			// HEREDOC-style syntax.
+			return strconv.Quote(v.AsString())
+		case cty.Number:
+			bf := v.AsBigFloat()
+			return bf.Text('g', -1)
+		case cty.Bool:
+			if v.True() {
+				return "true"
+			} else {
+				return "false"
+			}
+		}
+	case ty.IsObjectType():
+		return formatMappingValue(v, indent)
+	case ty.IsTupleType():
+		return formatSequenceValue(v, indent)
+	case ty.IsListType():
+		return fmt.Sprintf("tolist(%s)", formatSequenceValue(v, indent))
+	case ty.IsSetType():
+		return fmt.Sprintf("toset(%s)", formatSequenceValue(v, indent))
+	case ty.IsMapType():
+		return fmt.Sprintf("tomap(%s)", formatMappingValue(v, indent))
+	}
+
+	// Should never get here because there are no other types
+	return fmt.Sprintf("%#v", v)
 }
 
-func formatListResult(value []interface{}) (string, error) {
-	var outputBuf bytes.Buffer
-	outputBuf.WriteString("[")
-	if len(value) > 0 {
-		outputBuf.WriteString("\n")
+func formatMappingValue(v cty.Value, indent int) string {
+	var buf strings.Builder
+	count := 0
+	buf.WriteByte('{')
+	indent += 2
+	for it := v.ElementIterator(); it.Next(); {
+		count++
+		k, v := it.Element()
+		buf.WriteByte('\n')
+		buf.WriteString(strings.Repeat(" ", indent))
+		buf.WriteString(FormatValue(k, indent))
+		buf.WriteString(" = ")
+		buf.WriteString(FormatValue(v, indent))
 	}
-
-	for _, v := range value {
-		raw, err := formatResult(v, true)
-		if err != nil {
-			return "", err
-		}
-
-		outputBuf.WriteString(indent(raw))
-		outputBuf.WriteString(",\n")
+	indent -= 2
+	if count > 0 {
+		buf.WriteByte('\n')
+		buf.WriteString(strings.Repeat(" ", indent))
 	}
-
-	outputBuf.WriteString("]")
-	return outputBuf.String(), nil
+	buf.WriteByte('}')
+	return buf.String()
 }
 
-func formatMapResult(value map[string]interface{}) (string, error) {
-	ks := make([]string, 0, len(value))
-	for k, _ := range value {
-		ks = append(ks, k)
+func formatSequenceValue(v cty.Value, indent int) string {
+	var buf strings.Builder
+	count := 0
+	buf.WriteByte('[')
+	indent += 2
+	for it := v.ElementIterator(); it.Next(); {
+		count++
+		_, v := it.Element()
+		buf.WriteByte('\n')
+		buf.WriteString(strings.Repeat(" ", indent))
+		buf.WriteString(FormatValue(v, indent))
+		buf.WriteByte(',')
 	}
-	sort.Strings(ks)
-
-	var outputBuf bytes.Buffer
-	outputBuf.WriteString("{")
-	if len(value) > 0 {
-		outputBuf.WriteString("\n")
+	indent -= 2
+	if count > 0 {
+		buf.WriteByte('\n')
+		buf.WriteString(strings.Repeat(" ", indent))
 	}
-
-	for _, k := range ks {
-		v := value[k]
-		rawK, err := formatResult(k, true)
-		if err != nil {
-			return "", err
-		}
-		rawV, err := formatResult(v, true)
-		if err != nil {
-			return "", err
-		}
-
-		outputBuf.WriteString(indent(fmt.Sprintf("%s = %s", rawK, rawV)))
-		outputBuf.WriteString("\n")
-	}
-
-	outputBuf.WriteString("}")
-	return outputBuf.String(), nil
-}
-
-func indent(value string) string {
-	var outputBuf bytes.Buffer
-	s := bufio.NewScanner(strings.NewReader(value))
-	newline := false
-	for s.Scan() {
-		if newline {
-			outputBuf.WriteByte('\n')
-		}
-		outputBuf.WriteString("  " + s.Text())
-		newline = true
-	}
-
-	return outputBuf.String()
+	buf.WriteByte(']')
+	return buf.String()
 }

repl/format_test.go

@@ -0,0 +1,149 @@
+package repl
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/zclconf/go-cty/cty"
+)
+
+func TestFormatValue(t *testing.T) {
+	tests := []struct {
+		Val  cty.Value
+		Want string
+	}{
+		{
+			cty.NullVal(cty.DynamicPseudoType),
+			`null`,
+		},
+		{
+			cty.NullVal(cty.String),
+			`tostring(null)`,
+		},
+		{
+			cty.NullVal(cty.Number),
+			`tonumber(null)`,
+		},
+		{
+			cty.NullVal(cty.Bool),
+			`tobool(null)`,
+		},
+		{
+			cty.NullVal(cty.List(cty.String)),
+			`tolist(null) /* of string */`,
+		},
+		{
+			cty.NullVal(cty.Set(cty.Number)),
+			`toset(null) /* of number */`,
+		},
+		{
+			cty.NullVal(cty.Map(cty.Bool)),
+			`tomap(null) /* of bool */`,
+		},
+		{
+			cty.NullVal(cty.Object(map[string]cty.Type{"a": cty.Bool})),
+			`null /* object */`, // Ideally this would display the full object type, including its attributes
+		},
+		{
+			cty.UnknownVal(cty.DynamicPseudoType),
+			`(known after apply)`,
+		},
+		{
+			cty.StringVal(""),
+			`""`,
+		},
+		{
+			cty.StringVal("hello"),
+			`"hello"`,
+		},
+		{
+			cty.StringVal("hello\nworld"),
+			`"hello\nworld"`, // Ideally we'd use heredoc syntax here for better readability, but we don't yet
+		},
+		{
+			cty.Zero,
+			`0`,
+		},
+		{
+			cty.NumberIntVal(5),
+			`5`,
+		},
+		{
+			cty.NumberFloatVal(5.2),
+			`5.2`,
+		},
+		{
+			cty.False,
+			`false`,
+		},
+		{
+			cty.True,
+			`true`,
+		},
+		{
+			cty.EmptyObjectVal,
+			`{}`,
+		},
+		{
+			cty.ObjectVal(map[string]cty.Value{
+				"a": cty.StringVal("b"),
+			}),
+			`{
+  "a" = "b"
+}`,
+		},
+		{
+			cty.ObjectVal(map[string]cty.Value{
+				"a": cty.StringVal("b"),
+				"c": cty.StringVal("d"),
+			}),
+			`{
+  "a" = "b"
+  "c" = "d"
+}`,
+		},
+		{
+			cty.MapValEmpty(cty.String),
+			`tomap({})`,
+		},
+		{
+			cty.EmptyTupleVal,
+			`[]`,
+		},
+		{
+			cty.TupleVal([]cty.Value{
+				cty.StringVal("b"),
+			}),
+			`[
+  "b",
+]`,
+		},
+		{
+			cty.TupleVal([]cty.Value{
+				cty.StringVal("b"),
+				cty.StringVal("d"),
+			}),
+			`[
+  "b",
+  "d",
+]`,
+		},
+		{
+			cty.ListValEmpty(cty.String),
+			`tolist([])`,
+		},
+		{
+			cty.SetValEmpty(cty.String),
+			`toset([])`,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(fmt.Sprintf("%#v", test.Val), func(t *testing.T) {
+			got := FormatValue(test.Val, 0)
+			if got != test.Want {
+				t.Errorf("wrong result\nvalue: %#v\ngot:   %s\nwant:  %s", test.Val, got, test.Want)
+			}
+		})
+	}
+}

repl/session.go

@@ -2,14 +2,12 @@ package repl
 
 import (
 	"errors"
-	"fmt"
 	"strings"
 
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/hcl/v2/hclsyntax"
-	"github.com/hashicorp/terraform/configs/hcl2shim"
 	"github.com/hashicorp/terraform/lang"
 	"github.com/hashicorp/terraform/tfdiags"
 )
@@ -61,25 +59,7 @@ func (s *Session) handleEval(line string) (string, tfdiags.Diagnostics) {
 		return "", diags
 	}
 
-	if !val.IsWhollyKnown() {
-		// FIXME: In future, once we've updated the result formatter to be
-		// cty-aware, we should just include unknown values as "(not yet known)"
-		// in the serialized result, allowing the rest (if any) to be seen.
-		diags = diags.Append(fmt.Errorf("Result depends on values that cannot be determined until after \"terraform apply\"."))
-		return "", diags
-	}
-
-	// Our formatter still wants an old-style raw interface{} value, so
-	// for now we'll just shim it.
-	// FIXME: Port the formatter to work with cty.Value directly.
-	legacyVal := hcl2shim.ConfigValueFromHCL2(val)
-	result, err := FormatResult(legacyVal)
-	if err != nil {
-		diags = diags.Append(err)
-		return "", diags
-	}
-
-	return result, diags
+	return FormatValue(val, 0), diags
 }
 
 func (s *Session) handleHelp() (string, tfdiags.Diagnostics) {

repl/session_test.go

@@ -73,7 +73,7 @@ func TestSession_basicState(t *testing.T) {
 			Inputs: []testSessionInput{
 				{
 					Input:  "test_instance.foo.id",
-					Output: "bar",
+					Output: `"bar"`,
 				},
 			},
 		})