From 8b2b569d6e63a1b2969f67653edc38b559a2b08e Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Wed, 9 Sep 2020 14:13:53 -0400 Subject: [PATCH] repl: Improved value renderer for console outputs Use a slightly modified value renderer from terraform-provider-testing to display values in the console REPL, as well as outputs from the apply and outputs subcommands. Derived from code in this repository, MIT licensed: https://github.com/apparentlymart/terraform-provider-testing Note that this is technically a breaking change for the console subcommand, which would previously error if the user attempted to render an unknown value (such as an unset variable). This was marked as an unintentional side effect, with the goal being the new behaviour of rendering "(unknown)", which is why I changed the behaviour in this commit. --- command/apply.go | 13 +-- command/apply_test.go | 2 +- command/console_test.go | 31 +++---- command/output.go | 12 +-- command/output_test.go | 20 +++-- repl/format.go | 190 ++++++++++++++++++++-------------------- repl/format_test.go | 149 +++++++++++++++++++++++++++++++ repl/session.go | 22 +---- repl/session_test.go | 2 +- 9 files changed, 276 insertions(+), 165 deletions(-) create mode 100644 repl/format_test.go diff --git a/command/apply.go b/command/apply.go index f9ccddfc1..21063bbfc 100644 --- a/command/apply.go +++ b/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 = "" - } - + result := repl.FormatValue(v.Value, 0) outputBuf.WriteString(fmt.Sprintf("%s = %s\n", k, result)) } } diff --git a/command/apply_test.go b/command/apply_test.go index c4d0ba949..74719c32a 100644 --- a/command/apply_test.go +++ b/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 = ") { diff --git a/command/console_test.go b/command/console_test.go index b8e73fe33..bbfee6951 100644 --- a/command/console_test.go +++ b/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", } diff --git a/command/output.go b/command/output.go index a9d4441be..01f988cc6 100644 --- a/command/output.go +++ b/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) } diff --git a/command/output_test.go b/command/output_test.go index d8e4b862f..4ca121aa1 100644 --- a/command/output_test.go +++ b/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) } } diff --git a/repl/format.go b/repl/format.go index d8f95ea3d..051e2425e 100644 --- a/repl/format.go +++ b/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() } diff --git a/repl/format_test.go b/repl/format_test.go new file mode 100644 index 000000000..ca1d06232 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/repl/session.go b/repl/session.go index f24f3948d..1cb7028a3 100644 --- a/repl/session.go +++ b/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) { diff --git a/repl/session_test.go b/repl/session_test.go index 27eb10567..a49708276 100644 --- a/repl/session_test.go +++ b/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"`, }, }, })