command: Better visual hierarchy for diagnostics
I frequently see people attempting to ask questions about Terraform's error and warning messages but either only copying part of the message or accidentally copying a surrounding paragraph that isn't part of the message. While I'm sure some of these are just "careless" mistakes, I've also noticed that this has sometimes overlapped with someone asking a question whose answer is written directly in the part of the message they didn't include when copying, and so I have a theory that our current output doesn't create a good enough visual hierarchy for sighted users to understand where the diagnostic messages start and end when we show them in close proximity to other content, or to other diagnostic messages. As a result, some folks fail to notice the relevant message that might've answered their question. I tried a few different experiments for different approaches here, such as adding more horizontal rules to the output and coloring the detail text differently, but the approach that felt like the nicest compromise to me was what's implemented here, which is to add a vertical line along the left edge of each diagnostic message, colored to match with the typical color we use for each diagnostic severity. This means that the diagnostics end up slightly indented from what's around them, and the vertical line seems to help subtly signal how we intended the content to be grouped together.
This commit is contained in:
parent
ef7607df04
commit
e865faf318
|
@ -330,6 +330,7 @@ func TestInitProviderNotFound(t *testing.T) {
|
|||
|
||||
fixturePath := filepath.Join("testdata", "provider-not-found")
|
||||
tf := e2e.NewBinary(terraformBin, fixturePath)
|
||||
tf.AddEnv("TF_CLI_ARGS=-no-color")
|
||||
defer tf.Close()
|
||||
|
||||
t.Run("registry provider not found", func(t *testing.T) {
|
||||
|
@ -338,9 +339,9 @@ func TestInitProviderNotFound(t *testing.T) {
|
|||
t.Fatal("expected error, got success")
|
||||
}
|
||||
|
||||
oneLineStderr := strings.ReplaceAll(stderr, "\n", " ")
|
||||
oneLineStderr := strings.ReplaceAll(stderr, "\n│", "")
|
||||
if !strings.Contains(oneLineStderr, "provider registry registry.terraform.io does not have a provider named registry.terraform.io/hashicorp/nonexist") {
|
||||
t.Errorf("expected error message is missing from output:\n%s", stderr)
|
||||
t.Errorf("expected error message is missing from output:\n%s", oneLineStderr)
|
||||
}
|
||||
})
|
||||
|
||||
|
@ -356,8 +357,9 @@ func TestInitProviderNotFound(t *testing.T) {
|
|||
t.Fatal("expected error, got success")
|
||||
}
|
||||
|
||||
if !strings.Contains(stderr, "provider registry.terraform.io/hashicorp/nonexist was not\nfound in any of the search locations\n\n - "+pluginDir) {
|
||||
t.Errorf("expected error message is missing from output:\n%s", stderr)
|
||||
oneLineStderr := strings.ReplaceAll(stderr, "\n│", "")
|
||||
if !strings.Contains(oneLineStderr, "provider registry.terraform.io/hashicorp/nonexist was not found in any of the search locations - "+pluginDir) {
|
||||
t.Errorf("expected error message is missing from output:\n%s", oneLineStderr)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
@ -378,7 +380,7 @@ func TestInitProviderWarnings(t *testing.T) {
|
|||
t.Fatal("expected error, got success")
|
||||
}
|
||||
|
||||
if !strings.Contains(stdout, "This provider is archived and no longer needed. The terraform_remote_state\ndata source is built into the latest Terraform release.") {
|
||||
if !strings.Contains(stdout, "This provider is archived and no longer needed.") {
|
||||
t.Errorf("expected warning message is missing from output:\n%s", stdout)
|
||||
}
|
||||
|
||||
|
|
|
@ -31,11 +31,31 @@ func Diagnostic(diag tfdiags.Diagnostic, sources map[string][]byte, color *color
|
|||
|
||||
var buf bytes.Buffer
|
||||
|
||||
// these leftRule* variables are markers for the beginning of the lines
|
||||
// containing the diagnostic that are intended to help sighted users
|
||||
// better understand the information heirarchy when diagnostics appear
|
||||
// alongside other information or alongside other diagnostics.
|
||||
//
|
||||
// Without this, it seems (based on folks sharing incomplete messages when
|
||||
// asking questions, or including extra content that's not part of the
|
||||
// diagnostic) that some readers have trouble easily identifying which
|
||||
// text belongs to the diagnostic and which does not.
|
||||
var leftRuleLine, leftRuleStart, leftRuleEnd string
|
||||
var leftRuleWidth int // in visual character cells
|
||||
|
||||
switch diag.Severity() {
|
||||
case tfdiags.Error:
|
||||
buf.WriteString(color.Color("\n[bold][red]Error: [reset]"))
|
||||
buf.WriteString(color.Color("[bold][red]Error: [reset]"))
|
||||
leftRuleLine = color.Color("[red]│[reset] ")
|
||||
leftRuleStart = color.Color("[red]╷[reset]")
|
||||
leftRuleEnd = color.Color("[red]╵[reset]")
|
||||
leftRuleWidth = 2
|
||||
case tfdiags.Warning:
|
||||
buf.WriteString(color.Color("\n[bold][yellow]Warning: [reset]"))
|
||||
buf.WriteString(color.Color("[bold][yellow]Warning: [reset]"))
|
||||
leftRuleLine = color.Color("[yellow]│[reset] ")
|
||||
leftRuleStart = color.Color("[yellow]╷[reset]")
|
||||
leftRuleEnd = color.Color("[yellow]╵[reset]")
|
||||
leftRuleWidth = 2
|
||||
default:
|
||||
// Clear out any coloring that might be applied by Terraform's UI helper,
|
||||
// so our result is not context-sensitive.
|
||||
|
@ -183,11 +203,12 @@ func Diagnostic(diag tfdiags.Diagnostic, sources map[string][]byte, color *color
|
|||
}
|
||||
|
||||
if desc.Detail != "" {
|
||||
if width > 1 {
|
||||
paraWidth := width - leftRuleWidth - 1 // leave room for the left rule
|
||||
if paraWidth > 0 {
|
||||
lines := strings.Split(desc.Detail, "\n")
|
||||
for _, line := range lines {
|
||||
if !strings.HasPrefix(line, " ") {
|
||||
line = wordwrap.WrapString(line, uint(width-1))
|
||||
line = wordwrap.WrapString(line, uint(paraWidth))
|
||||
}
|
||||
fmt.Fprintf(&buf, "%s\n", line)
|
||||
}
|
||||
|
@ -196,7 +217,30 @@ func Diagnostic(diag tfdiags.Diagnostic, sources map[string][]byte, color *color
|
|||
}
|
||||
}
|
||||
|
||||
return buf.String()
|
||||
// Before we return, we'll finally add the left rule prefixes to each
|
||||
// line so that the overall message is visually delimited from what's
|
||||
// around it. We'll do that by scanning over what we already generated
|
||||
// and adding the prefix for each line.
|
||||
var ruleBuf strings.Builder
|
||||
sc := bufio.NewScanner(&buf)
|
||||
ruleBuf.WriteString(leftRuleStart)
|
||||
ruleBuf.WriteByte('\n')
|
||||
for sc.Scan() {
|
||||
line := sc.Text()
|
||||
prefix := leftRuleLine
|
||||
if line == "" {
|
||||
// Don't print the space after the line if there would be nothing
|
||||
// after it anyway.
|
||||
prefix = strings.TrimSpace(prefix)
|
||||
}
|
||||
ruleBuf.WriteString(prefix)
|
||||
ruleBuf.WriteString(line)
|
||||
ruleBuf.WriteByte('\n')
|
||||
}
|
||||
ruleBuf.WriteString(leftRuleEnd)
|
||||
ruleBuf.WriteByte('\n')
|
||||
|
||||
return ruleBuf.String()
|
||||
}
|
||||
|
||||
// DiagnosticWarningsCompact is an alternative to Diagnostic for when all of
|
||||
|
|
|
@ -25,12 +25,13 @@ func TestDiagnostic(t *testing.T) {
|
|||
"A sourceless error",
|
||||
"It has no source references but it does have a pretty long detail that should wrap over multiple lines.",
|
||||
),
|
||||
`
|
||||
[bold][red]Error: [reset][bold]A sourceless error[reset]
|
||||
|
||||
It has no source references but it does
|
||||
have a pretty long detail that should
|
||||
wrap over multiple lines.
|
||||
`[red]╷[reset]
|
||||
[red]│[reset] [bold][red]Error: [reset][bold]A sourceless error[reset]
|
||||
[red]│[reset]
|
||||
[red]│[reset] It has no source references but it
|
||||
[red]│[reset] does have a pretty long detail that
|
||||
[red]│[reset] should wrap over multiple lines.
|
||||
[red]╵[reset]
|
||||
`,
|
||||
},
|
||||
"sourceless warning": {
|
||||
|
@ -39,12 +40,13 @@ wrap over multiple lines.
|
|||
"A sourceless warning",
|
||||
"It has no source references but it does have a pretty long detail that should wrap over multiple lines.",
|
||||
),
|
||||
`
|
||||
[bold][yellow]Warning: [reset][bold]A sourceless warning[reset]
|
||||
|
||||
It has no source references but it does
|
||||
have a pretty long detail that should
|
||||
wrap over multiple lines.
|
||||
`[yellow]╷[reset]
|
||||
[yellow]│[reset] [bold][yellow]Warning: [reset][bold]A sourceless warning[reset]
|
||||
[yellow]│[reset]
|
||||
[yellow]│[reset] It has no source references but it
|
||||
[yellow]│[reset] does have a pretty long detail that
|
||||
[yellow]│[reset] should wrap over multiple lines.
|
||||
[yellow]╵[reset]
|
||||
`,
|
||||
},
|
||||
"error with source code subject": {
|
||||
|
@ -58,13 +60,14 @@ wrap over multiple lines.
|
|||
End: hcl.Pos{Line: 1, Column: 12, Byte: 11},
|
||||
},
|
||||
},
|
||||
`
|
||||
[bold][red]Error: [reset][bold]Bad bad bad[reset]
|
||||
|
||||
on test.tf line 1:
|
||||
1: test [underline]source[reset] code
|
||||
|
||||
Whatever shall we do?
|
||||
`[red]╷[reset]
|
||||
[red]│[reset] [bold][red]Error: [reset][bold]Bad bad bad[reset]
|
||||
[red]│[reset]
|
||||
[red]│[reset] on test.tf line 1:
|
||||
[red]│[reset] 1: test [underline]source[reset] code
|
||||
[red]│[reset]
|
||||
[red]│[reset] Whatever shall we do?
|
||||
[red]╵[reset]
|
||||
`,
|
||||
},
|
||||
"error with source code subject and known expression": {
|
||||
|
@ -89,15 +92,16 @@ Whatever shall we do?
|
|||
},
|
||||
},
|
||||
},
|
||||
`
|
||||
[bold][red]Error: [reset][bold]Bad bad bad[reset]
|
||||
|
||||
on test.tf line 1:
|
||||
1: test [underline]source[reset] code
|
||||
[dark_gray]├────────────────[reset]
|
||||
[dark_gray]│[reset] [bold]boop.beep[reset] is "blah"
|
||||
|
||||
Whatever shall we do?
|
||||
`[red]╷[reset]
|
||||
[red]│[reset] [bold][red]Error: [reset][bold]Bad bad bad[reset]
|
||||
[red]│[reset]
|
||||
[red]│[reset] on test.tf line 1:
|
||||
[red]│[reset] 1: test [underline]source[reset] code
|
||||
[red]│[reset] [dark_gray]├────────────────[reset]
|
||||
[red]│[reset] [dark_gray]│[reset] [bold]boop.beep[reset] is "blah"
|
||||
[red]│[reset]
|
||||
[red]│[reset] Whatever shall we do?
|
||||
[red]╵[reset]
|
||||
`,
|
||||
},
|
||||
"error with source code subject and expression referring to sensitive value": {
|
||||
|
@ -122,15 +126,16 @@ Whatever shall we do?
|
|||
},
|
||||
},
|
||||
},
|
||||
`
|
||||
[bold][red]Error: [reset][bold]Bad bad bad[reset]
|
||||
|
||||
on test.tf line 1:
|
||||
1: test [underline]source[reset] code
|
||||
[dark_gray]├────────────────[reset]
|
||||
[dark_gray]│[reset] [bold]boop.beep[reset] has a sensitive value
|
||||
|
||||
Whatever shall we do?
|
||||
`[red]╷[reset]
|
||||
[red]│[reset] [bold][red]Error: [reset][bold]Bad bad bad[reset]
|
||||
[red]│[reset]
|
||||
[red]│[reset] on test.tf line 1:
|
||||
[red]│[reset] 1: test [underline]source[reset] code
|
||||
[red]│[reset] [dark_gray]├────────────────[reset]
|
||||
[red]│[reset] [dark_gray]│[reset] [bold]boop.beep[reset] has a sensitive value
|
||||
[red]│[reset]
|
||||
[red]│[reset] Whatever shall we do?
|
||||
[red]╵[reset]
|
||||
`,
|
||||
},
|
||||
"error with source code subject and unknown string expression": {
|
||||
|
@ -155,15 +160,16 @@ Whatever shall we do?
|
|||
},
|
||||
},
|
||||
},
|
||||
`
|
||||
[bold][red]Error: [reset][bold]Bad bad bad[reset]
|
||||
|
||||
on test.tf line 1:
|
||||
1: test [underline]source[reset] code
|
||||
[dark_gray]├────────────────[reset]
|
||||
[dark_gray]│[reset] [bold]boop.beep[reset] is a string, known only after apply
|
||||
|
||||
Whatever shall we do?
|
||||
`[red]╷[reset]
|
||||
[red]│[reset] [bold][red]Error: [reset][bold]Bad bad bad[reset]
|
||||
[red]│[reset]
|
||||
[red]│[reset] on test.tf line 1:
|
||||
[red]│[reset] 1: test [underline]source[reset] code
|
||||
[red]│[reset] [dark_gray]├────────────────[reset]
|
||||
[red]│[reset] [dark_gray]│[reset] [bold]boop.beep[reset] is a string, known only after apply
|
||||
[red]│[reset]
|
||||
[red]│[reset] Whatever shall we do?
|
||||
[red]╵[reset]
|
||||
`,
|
||||
},
|
||||
"error with source code subject and unknown expression of unknown type": {
|
||||
|
@ -188,15 +194,16 @@ Whatever shall we do?
|
|||
},
|
||||
},
|
||||
},
|
||||
`
|
||||
[bold][red]Error: [reset][bold]Bad bad bad[reset]
|
||||
|
||||
on test.tf line 1:
|
||||
1: test [underline]source[reset] code
|
||||
[dark_gray]├────────────────[reset]
|
||||
[dark_gray]│[reset] [bold]boop.beep[reset] will be known only after apply
|
||||
|
||||
Whatever shall we do?
|
||||
`[red]╷[reset]
|
||||
[red]│[reset] [bold][red]Error: [reset][bold]Bad bad bad[reset]
|
||||
[red]│[reset]
|
||||
[red]│[reset] on test.tf line 1:
|
||||
[red]│[reset] 1: test [underline]source[reset] code
|
||||
[red]│[reset] [dark_gray]├────────────────[reset]
|
||||
[red]│[reset] [dark_gray]│[reset] [bold]boop.beep[reset] will be known only after apply
|
||||
[red]│[reset]
|
||||
[red]│[reset] Whatever shall we do?
|
||||
[red]╵[reset]
|
||||
`,
|
||||
},
|
||||
}
|
||||
|
@ -316,16 +323,17 @@ func TestDiagnostic_nonOverlappingHighlightContext(t *testing.T) {
|
|||
Reset: true,
|
||||
Disable: true,
|
||||
}
|
||||
expected := `
|
||||
Error: Some error
|
||||
|
||||
on source.tf line 1:
|
||||
1: x = somefunc("testing", {
|
||||
2: alpha = "foo"
|
||||
3: beta = "bar"
|
||||
4: })
|
||||
|
||||
...
|
||||
expected := `╷
|
||||
│ Error: Some error
|
||||
│
|
||||
│ on source.tf line 1:
|
||||
│ 1: x = somefunc("testing", {
|
||||
│ 2: alpha = "foo"
|
||||
│ 3: beta = "bar"
|
||||
│ 4: })
|
||||
│
|
||||
│ ...
|
||||
╵
|
||||
`
|
||||
output := Diagnostic(diags[0], sources, color, 80)
|
||||
|
||||
|
@ -364,15 +372,16 @@ func TestDiagnostic_emptyOverlapHighlightContext(t *testing.T) {
|
|||
Reset: true,
|
||||
Disable: true,
|
||||
}
|
||||
expected := `
|
||||
Error: Some error
|
||||
|
||||
on source.tf line 3, in variable "x":
|
||||
2: default = {
|
||||
3: "foo"
|
||||
4: }
|
||||
|
||||
...
|
||||
expected := `╷
|
||||
│ Error: Some error
|
||||
│
|
||||
│ on source.tf line 3, in variable "x":
|
||||
│ 2: default = {
|
||||
│ 3: "foo"
|
||||
│ 4: }
|
||||
│
|
||||
│ ...
|
||||
╵
|
||||
`
|
||||
output := Diagnostic(diags[0], sources, color, 80)
|
||||
|
||||
|
@ -394,17 +403,18 @@ func TestDiagnostic_wrapDetailIncludingCommand(t *testing.T) {
|
|||
Reset: true,
|
||||
Disable: true,
|
||||
}
|
||||
expected := `
|
||||
Error: Everything went wrong
|
||||
|
||||
This is a very long sentence about whatever went wrong which is supposed to
|
||||
wrap onto multiple lines. Thank-you very much for listening.
|
||||
|
||||
To fix this, run this very long command:
|
||||
terraform read-my-mind -please -thanks -but-do-not-wrap-this-line-because-it-is-prefixed-with-spaces
|
||||
|
||||
Here is a coda which is also long enough to wrap and so it should
|
||||
eventually make it onto multiple lines. THE END
|
||||
expected := `╷
|
||||
│ Error: Everything went wrong
|
||||
│
|
||||
│ This is a very long sentence about whatever went wrong which is supposed
|
||||
│ to wrap onto multiple lines. Thank-you very much for listening.
|
||||
│
|
||||
│ To fix this, run this very long command:
|
||||
│ terraform read-my-mind -please -thanks -but-do-not-wrap-this-line-because-it-is-prefixed-with-spaces
|
||||
│
|
||||
│ Here is a coda which is also long enough to wrap and so it should
|
||||
│ eventually make it onto multiple lines. THE END
|
||||
╵
|
||||
`
|
||||
output := Diagnostic(diags[0], nil, color, 76)
|
||||
|
||||
|
|
|
@ -659,11 +659,11 @@ func (m *Meta) showDiagnostics(vals ...interface{}) {
|
|||
msg := format.Diagnostic(diag, m.configSources(), m.Colorize(), outputWidth)
|
||||
switch diag.Severity() {
|
||||
case tfdiags.Error:
|
||||
m.Ui.Error(msg)
|
||||
m.Ui.Error(strings.TrimSpace(msg))
|
||||
case tfdiags.Warning:
|
||||
m.Ui.Warn(msg)
|
||||
m.Ui.Warn(strings.TrimSpace(msg))
|
||||
default:
|
||||
m.Ui.Output(msg)
|
||||
m.Ui.Output(strings.TrimSpace(msg))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
@ -290,18 +290,17 @@ func TestStateMv_resourceToInstanceErr(t *testing.T) {
|
|||
t.Fatalf("expected error output, got:\n%s", ui.OutputWriter.String())
|
||||
}
|
||||
|
||||
expectedErr := `
|
||||
Error: Invalid target address
|
||||
|
||||
Cannot move test_instance.foo to test_instance.bar[0]: the source is a whole
|
||||
resource (not a resource instance) so the target must also be a whole
|
||||
resource.
|
||||
|
||||
expectedErr := `╷
|
||||
│ Error: Invalid target address
|
||||
│
|
||||
│ Cannot move test_instance.foo to test_instance.bar[0]: the source is a
|
||||
│ whole resource (not a resource instance) so the target must also be a whole
|
||||
│ resource.
|
||||
╵
|
||||
`
|
||||
errOutput := ui.ErrorWriter.String()
|
||||
if errOutput != expectedErr {
|
||||
t.Errorf("Unexpected diff.\ngot:\n%s\nwant:\n%s\n", errOutput, expectedErr)
|
||||
t.Errorf("%s", cmp.Diff(errOutput, expectedErr))
|
||||
t.Errorf("wrong output\n%s", cmp.Diff(errOutput, expectedErr))
|
||||
}
|
||||
}
|
||||
func TestStateMv_instanceToResource(t *testing.T) {
|
||||
|
@ -503,9 +502,15 @@ func TestStateMv_differentResourceTypes(t *testing.T) {
|
|||
t.Fatalf("expected error output, got:\n%s", ui.OutputWriter.String())
|
||||
}
|
||||
|
||||
errOutput := strings.Replace(ui.ErrorWriter.String(), "\n", " ", -1)
|
||||
if !strings.Contains(errOutput, "resource types don't match") {
|
||||
t.Fatalf("expected initialization error, got:\n%s", ui.ErrorWriter.String())
|
||||
gotErr := strings.TrimSpace(ui.ErrorWriter.String())
|
||||
wantErr := strings.TrimSpace(`╷
|
||||
│ Error: Invalid state move request
|
||||
│
|
||||
│ Cannot move test_instance.foo to test_network.bar: resource types don't
|
||||
│ match.
|
||||
╵`)
|
||||
if gotErr != wantErr {
|
||||
t.Fatalf("expected initialization error\ngot:\n%s\n\nwant:%s", gotErr, wantErr)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -361,11 +361,12 @@ func TestTaint_missingAllow(t *testing.T) {
|
|||
|
||||
// Check for the warning
|
||||
actual := strings.TrimSpace(ui.ErrorWriter.String())
|
||||
expected := strings.TrimSpace(`
|
||||
Warning: No such resource instance
|
||||
|
||||
Resource instance test_instance.bar was not found, but this is not an error
|
||||
because -allow-missing was set.
|
||||
expected := strings.TrimSpace(`╷
|
||||
│ Warning: No such resource instance
|
||||
│
|
||||
│ Resource instance test_instance.bar was not found, but this is not an error
|
||||
│ because -allow-missing was set.
|
||||
╵
|
||||
`)
|
||||
if diff := cmp.Diff(expected, actual); diff != "" {
|
||||
t.Fatalf("wrong output\n%s", diff)
|
||||
|
|
|
@ -389,11 +389,12 @@ func TestUntaint_missingAllow(t *testing.T) {
|
|||
|
||||
// Check for the warning
|
||||
actual := strings.TrimSpace(ui.ErrorWriter.String())
|
||||
expected := strings.TrimSpace(`
|
||||
Warning: No such resource instance
|
||||
|
||||
Resource instance test_instance.bar was not found, but this is not an error
|
||||
because -allow-missing was set.
|
||||
expected := strings.TrimSpace(`╷
|
||||
│ Warning: No such resource instance
|
||||
│
|
||||
│ Resource instance test_instance.bar was not found, but this is not an error
|
||||
│ because -allow-missing was set.
|
||||
╵
|
||||
`)
|
||||
if diff := cmp.Diff(expected, actual); diff != "" {
|
||||
t.Fatalf("wrong output\n%s", diff)
|
||||
|
|
Loading…
Reference in New Issue