From c06675c616abaf4c9cfbd2afa18cc8360b4ad4e9 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 10 Dec 2019 11:06:06 -0800 Subject: [PATCH] command: New -compact-warnings option When warnings appear in isolation (not accompanied by an error) it's reasonable to want to defer resolving them for a while because they are not actually blocking immediate work. However, our warning messages tend to be long by default in order to include all of the necessary context to understand the implications of the warning, and that can make them overwhelming when combined with other output. As a compromise, this adds a new CLI option -compact-warnings which is supported for all the main operation commands and which uses a more compact format to print out warnings as long as they aren't also accompanied by errors. The default remains unchanged except that the threshold for consolidating warning messages is reduced to one so that we'll now only show one of each distinct warning summary. Full warning messages are always shown if there's at least one error included in the diagnostic set too, because in that case the warning message could contain additional context to help understand the error. --- command/apply.go | 6 +- command/format/diagnostic.go | 45 +++++++++++++ command/format/diagnostic_test.go | 73 +++++++++++++++++++++ command/meta.go | 32 ++++++++- command/plan.go | 4 ++ command/refresh.go | 4 ++ tfdiags/consolidate_warnings.go | 30 +++++++-- tfdiags/consolidate_warnings_test.go | 2 +- website/docs/commands/apply.html.markdown | 4 ++ website/docs/commands/plan.html.markdown | 4 ++ website/docs/commands/refresh.html.markdown | 4 ++ 11 files changed, 198 insertions(+), 10 deletions(-) create mode 100644 command/format/diagnostic_test.go diff --git a/command/apply.go b/command/apply.go index 28ddec774..76e500bf3 100644 --- a/command/apply.go +++ b/command/apply.go @@ -248,11 +248,15 @@ Usage: terraform apply [options] [DIR-OR-PLAN] Options: + -auto-approve Skip interactive approval of plan before applying. + -backup=path Path to backup the existing state file before modifying. Defaults to the "-state-out" path with ".backup" extension. Set to "-" to disable backup. - -auto-approve Skip interactive approval of plan before applying. + -compact-warnings If Terraform produces any warnings that are not + accompanied by errors, show them in a more compact + form that includes only the summary messages. -lock=true Lock the state file when locking is supported. diff --git a/command/format/diagnostic.go b/command/format/diagnostic.go index ed34ddbb9..6b8232650 100644 --- a/command/format/diagnostic.go +++ b/command/format/diagnostic.go @@ -177,6 +177,51 @@ func Diagnostic(diag tfdiags.Diagnostic, sources map[string][]byte, color *color return buf.String() } +// DiagnosticWarningsCompact is an alternative to Diagnostic for when all of +// the given diagnostics are warnings and we want to show them compactly, +// with only two lines per warning and excluding all of the detail information. +// +// The caller may optionally pre-process the given diagnostics with +// ConsolidateWarnings, in which case this function will recognize consolidated +// messages and include an indication that they are consolidated. +// +// Do not pass non-warning diagnostics to this function, or the result will +// be nonsense. +func DiagnosticWarningsCompact(diags tfdiags.Diagnostics, color *colorstring.Colorize) string { + var b strings.Builder + b.WriteString(color.Color("[bold][yellow]Warnings:[reset]\n\n")) + for _, diag := range diags { + sources := tfdiags.WarningGroupSourceRanges(diag) + b.WriteString(fmt.Sprintf("- %s\n", diag.Description().Summary)) + if len(sources) > 0 { + mainSource := sources[0] + if mainSource.Subject != nil { + if len(sources) > 1 { + b.WriteString(fmt.Sprintf( + " on %s line %d (and %d more)\n", + mainSource.Subject.Filename, + mainSource.Subject.Start.Line, + len(sources)-1, + )) + } else { + b.WriteString(fmt.Sprintf( + " on %s line %d\n", + mainSource.Subject.Filename, + mainSource.Subject.Start.Line, + )) + } + } else if len(sources) > 1 { + b.WriteString(fmt.Sprintf( + " (%d occurences of this warning)\n", + len(sources), + )) + } + } + } + + return b.String() +} + func parseRange(src []byte, rng hcl.Range) (*hcl.File, int) { filename := rng.Filename offset := rng.Start.Byte diff --git a/command/format/diagnostic_test.go b/command/format/diagnostic_test.go new file mode 100644 index 000000000..18812089c --- /dev/null +++ b/command/format/diagnostic_test.go @@ -0,0 +1,73 @@ +package format + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/hcl/v2" + "github.com/mitchellh/colorstring" + + "github.com/hashicorp/terraform/tfdiags" +) + +func TestDiagnosticWarningsCompact(t *testing.T) { + var diags tfdiags.Diagnostics + diags = diags.Append(tfdiags.SimpleWarning("foo")) + diags = diags.Append(tfdiags.SimpleWarning("foo")) + diags = diags.Append(tfdiags.SimpleWarning("bar")) + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "source foo", + Detail: "...", + Subject: &hcl.Range{ + Filename: "source.tf", + Start: hcl.Pos{Line: 2, Column: 1, Byte: 5}, + End: hcl.Pos{Line: 2, Column: 1, Byte: 5}, + }, + }) + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "source foo", + Detail: "...", + Subject: &hcl.Range{ + Filename: "source.tf", + Start: hcl.Pos{Line: 3, Column: 1, Byte: 7}, + End: hcl.Pos{Line: 3, Column: 1, Byte: 7}, + }, + }) + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "source bar", + Detail: "...", + Subject: &hcl.Range{ + Filename: "source2.tf", + Start: hcl.Pos{Line: 1, Column: 1, Byte: 1}, + End: hcl.Pos{Line: 1, Column: 1, Byte: 1}, + }, + }) + + // ConsolidateWarnings groups together the ones + // that have source location information and that + // have the same summary text. + diags = diags.ConsolidateWarnings(1) + + // A zero-value Colorize just passes all the formatting + // codes back to us, so we can test them literally. + got := DiagnosticWarningsCompact(diags, &colorstring.Colorize{}) + want := `[bold][yellow]Warnings:[reset] + +- foo +- foo +- bar +- source foo + on source.tf line 2 (and 1 more) +- source bar + on source2.tf line 1 +` + if got != want { + t.Errorf( + "wrong result\ngot:\n%s\n\nwant:\n%s\n\ndiff:\n%s", + got, want, cmp.Diff(want, got), + ) + } +} diff --git a/command/meta.go b/command/meta.go index 9f553da62..05009c999 100644 --- a/command/meta.go +++ b/command/meta.go @@ -157,6 +157,9 @@ type Meta struct { // init. // // reconfigure forces init to ignore any stored configuration. + // + // compactWarnings (-compact-warnings) selects a more compact presentation + // of warnings in the output when they are not accompanied by errors. statePath string stateOutPath string backupPath string @@ -166,6 +169,7 @@ type Meta struct { stateLockTimeout time.Duration forceInitCopy bool reconfigure bool + compactWarnings bool // Used with the import command to allow import of state when no matching config exists. allowMissingConfig bool @@ -377,6 +381,7 @@ func (m *Meta) extendedFlagSet(n string) *flag.FlagSet { f.BoolVar(&m.input, "input", true, "input") f.Var((*FlagTargetSlice)(&m.targets), "target", "resource to target") + f.BoolVar(&m.compactWarnings, "compact-warnings", false, "use compact warnings") if m.variableArgs.items == nil { m.variableArgs = newRawFlags("-var") @@ -480,8 +485,33 @@ func (m *Meta) showDiagnostics(vals ...interface{}) { diags = diags.Append(vals...) diags.Sort() + if len(diags) == 0 { + return + } + + diags = diags.ConsolidateWarnings(1) + // Since warning messages are generally competing - diags = diags.ConsolidateWarnings() + if m.compactWarnings { + // If the user selected compact warnings and all of the diagnostics are + // warnings then we'll use a more compact representation of the warnings + // that only includes their summaries. + // We show full warnings if there are also errors, because a warning + // can sometimes serve as good context for a subsequent error. + useCompact := true + for _, diag := range diags { + if diag.Severity() != tfdiags.Warning { + useCompact = false + break + } + } + if useCompact { + msg := format.DiagnosticWarningsCompact(diags, m.Colorize()) + msg = "\n" + msg + "\nTo see the full warning notes, run Terraform without -compact-warnings.\n" + m.Ui.Warn(msg) + return + } + } for _, diag := range diags { // TODO: Actually measure the terminal width and pass it here. diff --git a/command/plan.go b/command/plan.go index aad035705..e3e181566 100644 --- a/command/plan.go +++ b/command/plan.go @@ -201,6 +201,10 @@ Usage: terraform plan [options] [DIR] Options: + -compact-warnings If Terraform produces any warnings that are not + accompanied by errors, show them in a more compact form + that includes only the summary messages. + -destroy If set, a plan will be generated to destroy all resources managed by the given configuration and state. diff --git a/command/refresh.go b/command/refresh.go index db7ca9c43..dbb4ab61e 100644 --- a/command/refresh.go +++ b/command/refresh.go @@ -124,6 +124,10 @@ Options: modifying. Defaults to the "-state-out" path with ".backup" extension. Set to "-" to disable backup. + -compact-warnings If Terraform produces any warnings that are not + accompanied by errors, show them in a more compact form + that includes only the summary messages. + -input=true Ask for input for variables if not directly set. -lock=true Lock the state file when locking is supported. diff --git a/tfdiags/consolidate_warnings.go b/tfdiags/consolidate_warnings.go index 3e0983ee4..06f3d52cc 100644 --- a/tfdiags/consolidate_warnings.go +++ b/tfdiags/consolidate_warnings.go @@ -15,12 +15,9 @@ import "fmt" // The returned slice always has a separate backing array from the reciever, // but some diagnostic values themselves might be shared. // -// The definition of "unreasonable" may change in future releases. -func (diags Diagnostics) ConsolidateWarnings() Diagnostics { - // We'll start grouping when there are more than this number of warnings - // with the same summary. - const unreasonableThreshold = 2 - +// The definition of "unreasonable" is given as the threshold argument. At most +// that many warnings with the same summary will be shown. +func (diags Diagnostics) ConsolidateWarnings(threshold int) Diagnostics { if len(diags) == 0 { return nil } @@ -55,7 +52,7 @@ func (diags Diagnostics) ConsolidateWarnings() Diagnostics { } warningStats[summary]++ - if warningStats[summary] == unreasonableThreshold { + if warningStats[summary] == threshold { // Initially creating the group doesn't really change anything // visibly in the result, since a group with only one warning // is just a passthrough anyway, but once we do this any additional @@ -128,3 +125,22 @@ func (wg *warningGroup) Append(diag Diagnostic) { } wg.Warnings = append(wg.Warnings, diag) } + +// WarningGroupSourceRanges can be used in conjunction with +// Diagnostics.ConsolidateWarnings to recover the full set of original source +// locations from a consolidated warning. +// +// For convenience, this function accepts any diagnostic and will just return +// the single Source value from any diagnostic that isn't a warning group. +func WarningGroupSourceRanges(diag Diagnostic) []Source { + wg, ok := diag.(*warningGroup) + if !ok { + return []Source{diag.Source()} + } + + ret := make([]Source, len(wg.Warnings)) + for i, wrappedDiag := range wg.Warnings { + ret[i] = wrappedDiag.Source() + } + return ret +} diff --git a/tfdiags/consolidate_warnings_test.go b/tfdiags/consolidate_warnings_test.go index 6eb7eabc3..df94d4af8 100644 --- a/tfdiags/consolidate_warnings_test.go +++ b/tfdiags/consolidate_warnings_test.go @@ -53,7 +53,7 @@ func TestConsolidateWarnings(t *testing.T) { // We're using ForRPC here to force the diagnostics to be of a consistent // type that we can easily assert against below. - got := diags.ConsolidateWarnings().ForRPC() + got := diags.ConsolidateWarnings(2).ForRPC() want := Diagnostics{ // First set &rpcFriendlyDiag{ diff --git a/website/docs/commands/apply.html.markdown b/website/docs/commands/apply.html.markdown index 11c372469..239e16cfb 100644 --- a/website/docs/commands/apply.html.markdown +++ b/website/docs/commands/apply.html.markdown @@ -27,6 +27,10 @@ The command-line flags are all optional. The list of available flags are: * `-backup=path` - Path to the backup file. Defaults to `-state-out` with the ".backup" extension. Disabled by setting to "-". +* `-compact-warnings` - If Terraform produces any warnings that are not + accompanied by errors, show them in a more compact form that includes only + the summary messages. + * `-lock=true` - Lock the state file when locking is supported. * `-lock-timeout=0s` - Duration to retry a state lock. diff --git a/website/docs/commands/plan.html.markdown b/website/docs/commands/plan.html.markdown index 838daf7a2..815e237ea 100644 --- a/website/docs/commands/plan.html.markdown +++ b/website/docs/commands/plan.html.markdown @@ -37,6 +37,10 @@ inspect a planfile. The command-line flags are all optional. The list of available flags are: +* `-compact-warnings` - If Terraform produces any warnings that are not + accompanied by errors, show them in a more compact form that includes only + the summary messages. + * `-destroy` - If set, generates a plan to destroy all the known resources. * `-detailed-exitcode` - Return a detailed exit code when the command exits. diff --git a/website/docs/commands/refresh.html.markdown b/website/docs/commands/refresh.html.markdown index c03e68911..4700bf3e9 100644 --- a/website/docs/commands/refresh.html.markdown +++ b/website/docs/commands/refresh.html.markdown @@ -29,6 +29,10 @@ The command-line flags are all optional. The list of available flags are: * `-backup=path` - Path to the backup file. Defaults to `-state-out` with the ".backup" extension. Disabled by setting to "-". +* `-compact-warnings` - If Terraform produces any warnings that are not + accompanied by errors, show them in a more compact form that includes only + the summary messages. + * `-input=true` - Ask for input for variables if not directly set. * `-lock=true` - Lock the state file when locking is supported.