From 536c80da23c96e9a06acb2a9dbbf7fabd18091fd Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Fri, 12 Feb 2021 13:59:14 -0500 Subject: [PATCH] backend: Add per-operation diagnostic rendering The enhanced backends (local and remote) need to be able to render diagnostics during operations. Prior to this commit, this functionality was supported with a per-backend `ShowDiagnostics` function pointer. In order to allow users of these backends to control how diagnostics are rendered, this commit moves that function pointer to the `Operation` type. This means that a diagnostic renderer is configured for each operation, rather than once per backend initialization. Some secondary consequences of this change: - The `ReportResult` method on the backend is now moved to the `Operation` type, as it needs to access the `ShowDiagnostics` callback (and nothing else from the backend); - Tests which assumed that diagnostics would be written to the backend's `cli.Ui` instance are migrated to using a new record/playback diags helper function; - Apply, plan, and refresh commands now pass a pointer to the `Meta` struct's `showDiagnostics` method. This commit should not change how Terraform works, and is refactoring in preparation for more changes which move UI code out of the backend. --- backend/backend.go | 37 ++++++++++++++ backend/local/backend.go | 36 -------------- backend/local/backend_apply.go | 18 +++---- backend/local/backend_apply_test.go | 7 +-- backend/local/backend_plan.go | 18 +++---- backend/local/backend_plan_test.go | 14 ++++-- backend/local/backend_refresh.go | 10 ++-- backend/local/backend_refresh_test.go | 9 ++-- backend/local/cli.go | 1 - backend/local/testing.go | 60 ++++++++++++++-------- backend/remote/backend.go | 52 ++++---------------- backend/remote/backend_apply_test.go | 71 ++++++++++++++++----------- backend/remote/backend_plan_test.go | 63 ++++++++++++++---------- backend/remote/cli.go | 1 - backend/remote/testing.go | 48 +++++++++++++++--- command/apply.go | 1 + command/meta_backend.go | 1 - command/plan.go | 1 + command/refresh.go | 1 + 19 files changed, 250 insertions(+), 199 deletions(-) diff --git a/backend/backend.go b/backend/backend.go index b929aa393..73d529ca1 100644 --- a/backend/backend.go +++ b/backend/backend.go @@ -8,6 +8,7 @@ import ( "context" "errors" "io/ioutil" + "log" "os" "time" @@ -208,6 +209,9 @@ type Operation struct { UIIn terraform.UIInput UIOut terraform.UIOutput + // ShowDiagnostics prints diagnostic messages to the UI. + ShowDiagnostics func(vals ...interface{}) + // If LockState is true, the Operation must Lock any // statemgr.Lockers for its duration, and Unlock when complete. LockState bool @@ -240,6 +244,39 @@ func (o *Operation) Config() (*configs.Config, tfdiags.Diagnostics) { return config, diags } +// ReportResult is a helper for the common chore of setting the status of +// a running operation and showing any diagnostics produced during that +// operation. +// +// If the given diagnostics contains errors then the operation's result +// will be set to backend.OperationFailure. It will be set to +// backend.OperationSuccess otherwise. It will then use b.ShowDiagnostics +// to show the given diagnostics before returning. +// +// Callers should feel free to do each of these operations separately in +// more complex cases where e.g. diagnostics are interleaved with other +// output, but terminating immediately after reporting error diagnostics is +// common and can be expressed concisely via this method. +func (o *Operation) ReportResult(op *RunningOperation, diags tfdiags.Diagnostics) { + if diags.HasErrors() { + op.Result = OperationFailure + } else { + op.Result = OperationSuccess + } + if o.ShowDiagnostics != nil { + o.ShowDiagnostics(diags) + } else { + // Shouldn't generally happen, but if it does then we'll at least + // make some noise in the logs to help us spot it. + if len(diags) != 0 { + log.Printf( + "[ERROR] Backend needs to report diagnostics but ShowDiagnostics is not set:\n%s", + diags.ErrWithWarnings(), + ) + } + } +} + // RunningOperation is the result of starting an operation. type RunningOperation struct { // For implementers of a backend, this context should not wrap the diff --git a/backend/local/backend.go b/backend/local/backend.go index 8acbb0054..a82013b51 100644 --- a/backend/local/backend.go +++ b/backend/local/backend.go @@ -43,9 +43,6 @@ type Local struct { // input/output handles that CLI is connected to. Streams *terminal.Streams - // ShowDiagnostics prints diagnostic messages to the UI. - ShowDiagnostics func(vals ...interface{}) - // The State* paths are set from the backend config, and may be left blank // to use the defaults. If the actual paths for the local backend state are // needed, use the StatePaths method. @@ -398,39 +395,6 @@ func (b *Local) opWait( return } -// ReportResult is a helper for the common chore of setting the status of -// a running operation and showing any diagnostics produced during that -// operation. -// -// If the given diagnostics contains errors then the operation's result -// will be set to backend.OperationFailure. It will be set to -// backend.OperationSuccess otherwise. It will then use b.ShowDiagnostics -// to show the given diagnostics before returning. -// -// Callers should feel free to do each of these operations separately in -// more complex cases where e.g. diagnostics are interleaved with other -// output, but terminating immediately after reporting error diagnostics is -// common and can be expressed concisely via this method. -func (b *Local) ReportResult(op *backend.RunningOperation, diags tfdiags.Diagnostics) { - if diags.HasErrors() { - op.Result = backend.OperationFailure - } else { - op.Result = backend.OperationSuccess - } - if b.ShowDiagnostics != nil { - b.ShowDiagnostics(diags) - } else { - // Shouldn't generally happen, but if it does then we'll at least - // make some noise in the logs to help us spot it. - if len(diags) != 0 { - log.Printf( - "[ERROR] Local backend needs to report diagnostics but ShowDiagnostics is not set:\n%s", - diags.ErrWithWarnings(), - ) - } - } -} - // Colorize returns the Colorize structure that can be used for colorizing // output. This is guaranteed to always return a non-nil value and so is useful // as a helper to wrap any potentially colored strings. diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index cd018269a..936f9fe99 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -35,7 +35,7 @@ func (b *Local) opApply( "would mark everything for destruction, which is normally not what is desired. "+ "If you would like to destroy everything, run 'terraform destroy' instead.", )) - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } @@ -51,7 +51,7 @@ func (b *Local) opApply( tfCtx, _, opState, contextDiags := b.context(op) diags = diags.Append(contextDiags) if contextDiags.HasErrors() { - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } // the state was locked during succesfull context creation; unlock the state @@ -59,7 +59,7 @@ func (b *Local) opApply( defer func() { err := op.StateLocker.Unlock(nil) if err != nil { - b.ShowDiagnostics(err) + op.ShowDiagnostics(err) runningOp.Result = backend.OperationFailure } }() @@ -73,7 +73,7 @@ func (b *Local) opApply( plan, planDiags := tfCtx.Plan() diags = diags.Append(planDiags) if planDiags.HasErrors() { - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } @@ -109,7 +109,7 @@ func (b *Local) opApply( // We'll show any accumulated warnings before we display the prompt, // so the user can consider them when deciding how to answer. if len(diags) > 0 { - b.ShowDiagnostics(diags) + op.ShowDiagnostics(diags) diags = nil // reset so we won't show the same diagnostics again later } @@ -120,7 +120,7 @@ func (b *Local) opApply( }) if err != nil { diags = diags.Append(errwrap.Wrapf("Error asking for approval: {{err}}", err)) - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } if v != "yes" { @@ -167,20 +167,20 @@ func (b *Local) opApply( stateFile.State = applyState diags = diags.Append(b.backupStateForError(stateFile, err)) - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } diags = diags.Append(applyDiags) if applyDiags.HasErrors() { - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } // If we've accumulated any warnings along the way then we'll show them // here just before we show the summary and next steps. If we encountered // errors then we would've returned early at some other point above. - b.ShowDiagnostics(diags) + op.ShowDiagnostics(diags) } // backupStateForError is called in a scenario where we're unable to persist the diff --git a/backend/local/backend_apply_test.go b/backend/local/backend_apply_test.go index 106853e7c..1fe762248 100644 --- a/backend/local/backend_apply_test.go +++ b/backend/local/backend_apply_test.go @@ -289,9 +289,10 @@ func testOperationApply(t *testing.T, configDir string) (*backend.Operation, fun _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) return &backend.Operation{ - Type: backend.OperationTypeApply, - ConfigDir: configDir, - ConfigLoader: configLoader, + Type: backend.OperationTypeApply, + ConfigDir: configDir, + ConfigLoader: configLoader, + ShowDiagnostics: testLogDiagnostics(t), }, configCleanup } diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go index 3a45ec607..50ea3420b 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -41,7 +41,7 @@ func (b *Local) opPlan( "The plan command was given a saved plan file as its input. This command generates "+ "a new plan, and so it requires a configuration directory as its argument.", )) - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } @@ -55,7 +55,7 @@ func (b *Local) opPlan( "would like to destroy everything, run plan with the -destroy option. Otherwise, "+ "create a Terraform configuration file (.tf file) and try again.", )) - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } @@ -67,7 +67,7 @@ func (b *Local) opPlan( tfCtx, configSnap, opState, ctxDiags := b.context(op) diags = diags.Append(ctxDiags) if ctxDiags.HasErrors() { - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } // the state was locked during succesfull context creation; unlock the state @@ -75,7 +75,7 @@ func (b *Local) opPlan( defer func() { err := op.StateLocker.Unlock(nil) if err != nil { - b.ShowDiagnostics(err) + op.ShowDiagnostics(err) runningOp.Result = backend.OperationFailure } }() @@ -103,7 +103,7 @@ func (b *Local) opPlan( diags = diags.Append(planDiags) if planDiags.HasErrors() { - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } @@ -118,7 +118,7 @@ func (b *Local) opPlan( diags = diags.Append(fmt.Errorf( "PlanOutPath set without also setting PlanOutBackend (this is a bug in Terraform)"), ) - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } plan.Backend = *op.PlanOutBackend @@ -136,7 +136,7 @@ func (b *Local) opPlan( "Failed to write plan file", fmt.Sprintf("The plan file could not be written: %s.", err), )) - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } } @@ -149,7 +149,7 @@ func (b *Local) opPlan( b.CLI.Output("\n" + b.Colorize().Color(strings.TrimSpace(planNoChanges))) b.CLI.Output("\n" + strings.TrimSpace(format.WordWrap(planNoChangesDetail, outputColumns))) // Even if there are no changes, there still could be some warnings - b.ShowDiagnostics(diags) + op.ShowDiagnostics(diags) return } @@ -158,7 +158,7 @@ func (b *Local) opPlan( // If we've accumulated any warnings along the way then we'll show them // here just before we show the summary and next steps. If we encountered // errors then we would've returned early at some other point above. - b.ShowDiagnostics(diags) + op.ShowDiagnostics(diags) // Give the user some next-steps, unless we're running in an automation // tool which is presumed to provide its own UI for further actions. diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go index 4e0bc85a3..5e7b06ead 100644 --- a/backend/local/backend_plan_test.go +++ b/backend/local/backend_plan_test.go @@ -114,6 +114,8 @@ func TestLocal_planNoConfig(t *testing.T) { b.CLI = cli.NewMockUi() op, configCleanup := testOperationPlan(t, "./testdata/empty") + record, playback := testRecordDiagnostics(t) + op.ShowDiagnostics = record defer configCleanup() op.PlanRefresh = true @@ -126,8 +128,9 @@ func TestLocal_planNoConfig(t *testing.T) { if run.Result == backend.OperationSuccess { t.Fatal("plan operation succeeded; want failure") } - output := b.CLI.(*cli.MockUi).ErrorWriter.String() - if !strings.Contains(output, "configuration") { + + output := playback().Err().Error() + if !strings.Contains(output, "No configuration files") { t.Fatalf("bad: %s", err) } @@ -733,9 +736,10 @@ func testOperationPlan(t *testing.T, configDir string) (*backend.Operation, func _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) return &backend.Operation{ - Type: backend.OperationTypePlan, - ConfigDir: configDir, - ConfigLoader: configLoader, + Type: backend.OperationTypePlan, + ConfigDir: configDir, + ConfigLoader: configLoader, + ShowDiagnostics: testLogDiagnostics(t), }, configCleanup } diff --git a/backend/local/backend_refresh.go b/backend/local/backend_refresh.go index af6058dc7..83ce3ef31 100644 --- a/backend/local/backend_refresh.go +++ b/backend/local/backend_refresh.go @@ -36,7 +36,7 @@ func (b *Local) opRefresh( "Cannot read state file", fmt.Sprintf("Failed to read %s: %s", b.StatePath, err), )) - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } } @@ -49,7 +49,7 @@ func (b *Local) opRefresh( tfCtx, _, opState, contextDiags := b.context(op) diags = diags.Append(contextDiags) if contextDiags.HasErrors() { - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } @@ -58,7 +58,7 @@ func (b *Local) opRefresh( defer func() { err := op.StateLocker.Unlock(nil) if err != nil { - b.ShowDiagnostics(err) + op.ShowDiagnostics(err) runningOp.Result = backend.OperationFailure } }() @@ -94,14 +94,14 @@ func (b *Local) opRefresh( runningOp.State = newState diags = diags.Append(refreshDiags) if refreshDiags.HasErrors() { - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } err := statemgr.WriteAndPersist(opState, newState) if err != nil { diags = diags.Append(errwrap.Wrapf("Failed to write state: {{err}}", err)) - b.ReportResult(runningOp, diags) + op.ReportResult(runningOp, diags) return } } diff --git a/backend/local/backend_refresh_test.go b/backend/local/backend_refresh_test.go index 6062ed0d6..14210f790 100644 --- a/backend/local/backend_refresh_test.go +++ b/backend/local/backend_refresh_test.go @@ -220,10 +220,11 @@ func testOperationRefresh(t *testing.T, configDir string) (*backend.Operation, f _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) return &backend.Operation{ - Type: backend.OperationTypeRefresh, - ConfigDir: configDir, - ConfigLoader: configLoader, - LockState: true, + Type: backend.OperationTypeRefresh, + ConfigDir: configDir, + ConfigLoader: configLoader, + LockState: true, + ShowDiagnostics: testLogDiagnostics(t), }, configCleanup } diff --git a/backend/local/cli.go b/backend/local/cli.go index 432ffc383..c698c0af9 100644 --- a/backend/local/cli.go +++ b/backend/local/cli.go @@ -12,7 +12,6 @@ func (b *Local) CLIInit(opts *backend.CLIOpts) error { b.CLI = opts.CLI b.CLIColor = opts.CLIColor b.Streams = opts.Streams - b.ShowDiagnostics = opts.ShowDiagnostics b.ContextOpts = opts.ContextOpts b.OpInput = opts.Input b.OpValidation = opts.Validation diff --git a/backend/local/testing.go b/backend/local/testing.go index ab2246454..6e100fcc3 100644 --- a/backend/local/testing.go +++ b/backend/local/testing.go @@ -34,26 +34,6 @@ func TestLocal(t *testing.T) (*Local, func()) { local.StateWorkspaceDir = filepath.Join(tempDir, "state.tfstate.d") local.ContextOpts = &terraform.ContextOpts{} - local.ShowDiagnostics = func(vals ...interface{}) { - var diags tfdiags.Diagnostics - diags = diags.Append(vals...) - for _, diag := range diags { - // NOTE: Since the caller here is not directly the TestLocal - // function, t.Helper doesn't apply and so the log source - // isn't correctly shown in the test log output. This seems - // unavoidable as long as this is happening so indirectly. - desc := diag.Description() - if desc.Detail != "" { - t.Logf("%s: %s", desc.Summary, desc.Detail) - } else { - t.Log(desc.Summary) - } - if local.CLI != nil { - local.CLI.Error(desc.Summary) - } - } - } - cleanup := func() { if err := os.RemoveAll(tempDir); err != nil { t.Fatal("error cleanup up test:", err) @@ -265,3 +245,43 @@ func assertBackendStateLocked(t *testing.T, b *Local) bool { t.Error("unexpected success locking state") return true } + +// testRecordDiagnostics allows tests to record and later inspect diagnostics +// emitted during an Operation. It returns a record function which can be set +// as the ShowDiagnostics value of an Operation, and a playback function which +// returns the recorded diagnostics for inspection. +func testRecordDiagnostics(t *testing.T) (record func(vals ...interface{}), playback func() tfdiags.Diagnostics) { + var diags tfdiags.Diagnostics + record = func(vals ...interface{}) { + diags = diags.Append(vals...) + } + playback = func() tfdiags.Diagnostics { + diags.Sort() + return diags + } + return +} + +// testLogDiagnostics returns a function which can be used as the +// ShowDiagnostics value for an Operation, in order to help debugging during +// tests. Any calls to this function result in test logs. +func testLogDiagnostics(t *testing.T) func(vals ...interface{}) { + return func(vals ...interface{}) { + var diags tfdiags.Diagnostics + diags = diags.Append(vals...) + diags.Sort() + + for _, diag := range diags { + // NOTE: Since the caller here is not directly the TestLocal + // function, t.Helper doesn't apply and so the log source + // isn't correctly shown in the test log output. This seems + // unavoidable as long as this is happening so indirectly. + desc := diag.Description() + if desc.Detail != "" { + t.Logf("%s: %s", desc.Summary, desc.Detail) + } else { + t.Log(desc.Summary) + } + } + } +} diff --git a/backend/remote/backend.go b/backend/remote/backend.go index c79ff32e8..e3ab299fb 100644 --- a/backend/remote/backend.go +++ b/backend/remote/backend.go @@ -45,9 +45,6 @@ type Remote struct { CLI cli.Ui CLIColor *colorstring.Colorize - // ShowDiagnostics prints diagnostic messages to the UI. - ShowDiagnostics func(vals ...interface{}) - // ContextOpts are the base context options to set when initializing a // new Terraform context. Many of these will be overridden or merged by // Operation. See Operation for more details. @@ -755,7 +752,9 @@ func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend r, opErr := f(stopCtx, cancelCtx, op, w) if opErr != nil && opErr != context.Canceled { - b.ReportResult(runningOp, opErr) + var diags tfdiags.Diagnostics + diags = diags.Append(opErr) + op.ReportResult(runningOp, diags) return } @@ -768,7 +767,9 @@ func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend // Retrieve the run to get its current status. r, err := b.client.Runs.Read(cancelCtx, r.ID) if err != nil { - b.ReportResult(runningOp, generalError("Failed to retrieve run", err)) + var diags tfdiags.Diagnostics + diags = diags.Append(generalError("Failed to retrieve run", err)) + op.ReportResult(runningOp, diags) return } @@ -777,7 +778,9 @@ func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend if opErr == context.Canceled { if err := b.cancel(cancelCtx, op, r); err != nil { - b.ReportResult(runningOp, generalError("Failed to retrieve run", err)) + var diags tfdiags.Diagnostics + diags = diags.Append(generalError("Failed to retrieve run", err)) + op.ReportResult(runningOp, diags) return } } @@ -831,43 +834,6 @@ func (b *Remote) cancel(cancelCtx context.Context, op *backend.Operation, r *tfe return nil } -// ReportResult is a helper for the common chore of setting the status of -// a running operation and showing any diagnostics produced during that -// operation. -// -// If the given diagnostics contains errors then the operation's result -// will be set to backend.OperationFailure. It will be set to -// backend.OperationSuccess otherwise. It will then use b.ShowDiagnostics -// to show the given diagnostics before returning. -// -// Callers should feel free to do each of these operations separately in -// more complex cases where e.g. diagnostics are interleaved with other -// output, but terminating immediately after reporting error diagnostics is -// common and can be expressed concisely via this method. -func (b *Remote) ReportResult(op *backend.RunningOperation, err error) { - var diags tfdiags.Diagnostics - - diags = diags.Append(err) - if diags.HasErrors() { - op.Result = backend.OperationFailure - } else { - op.Result = backend.OperationSuccess - } - - if b.ShowDiagnostics != nil { - b.ShowDiagnostics(diags) - } else { - // Shouldn't generally happen, but if it does then we'll at least - // make some noise in the logs to help us spot it. - if len(diags) != 0 { - log.Printf( - "[ERROR] Remote backend needs to report diagnostics but ShowDiagnostics is not set:\n%s", - diags.ErrWithWarnings(), - ) - } - } -} - // IgnoreVersionConflict allows commands to disable the fall-back check that // the local Terraform version matches the remote workspace's configured // Terraform version. This should be called by commands where this check is diff --git a/backend/remote/backend_apply_test.go b/backend/remote/backend_apply_test.go index c39587867..54e3afd74 100644 --- a/backend/remote/backend_apply_test.go +++ b/backend/remote/backend_apply_test.go @@ -18,6 +18,7 @@ import ( "github.com/hashicorp/terraform/plans/planfile" "github.com/hashicorp/terraform/states/statemgr" "github.com/hashicorp/terraform/terraform" + "github.com/hashicorp/terraform/tfdiags" tfversion "github.com/hashicorp/terraform/version" "github.com/mitchellh/cli" ) @@ -28,14 +29,26 @@ func testOperationApply(t *testing.T, configDir string) (*backend.Operation, fun _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) return &backend.Operation{ - ConfigDir: configDir, - ConfigLoader: configLoader, - Parallelism: defaultParallelism, - PlanRefresh: true, - Type: backend.OperationTypeApply, + ConfigDir: configDir, + ConfigLoader: configLoader, + Parallelism: defaultParallelism, + PlanRefresh: true, + ShowDiagnostics: testLogDiagnostics(t), + Type: backend.OperationTypeApply, }, configCleanup } +func testOperationApplyWithDiagnostics(t *testing.T, configDir string) (*backend.Operation, func(), func() tfdiags.Diagnostics) { + t.Helper() + + op, cleanup := testOperationApply(t, configDir) + + record, playback := testRecordDiagnostics(t) + op.ShowDiagnostics = record + + return op, cleanup, playback +} + func TestRemote_applyBasic(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() @@ -131,7 +144,7 @@ func TestRemote_applyWithoutPermissions(t *testing.T) { } w.Permissions.CanQueueApply = false - op, configCleanup := testOperationApply(t, "./testdata/apply") + op, configCleanup, playback := testOperationApplyWithDiagnostics(t, "./testdata/apply") defer configCleanup() op.UIOut = b.CLI @@ -147,7 +160,7 @@ func TestRemote_applyWithoutPermissions(t *testing.T) { t.Fatal("expected apply operation to fail") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "Insufficient rights to apply changes") { t.Fatalf("expected a permissions error, got: %v", errOutput) } @@ -170,7 +183,7 @@ func TestRemote_applyWithVCS(t *testing.T) { t.Fatalf("error creating named workspace: %v", err) } - op, configCleanup := testOperationApply(t, "./testdata/apply") + op, configCleanup, playback := testOperationApplyWithDiagnostics(t, "./testdata/apply") defer configCleanup() op.Workspace = "prod" @@ -188,7 +201,7 @@ func TestRemote_applyWithVCS(t *testing.T) { t.Fatalf("expected plan to be empty") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "not allowed for workspaces with a VCS") { t.Fatalf("expected a VCS error, got: %v", errOutput) } @@ -198,7 +211,7 @@ func TestRemote_applyWithParallelism(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationApply(t, "./testdata/apply") + op, configCleanup, playback := testOperationApplyWithDiagnostics(t, "./testdata/apply") defer configCleanup() op.Parallelism = 3 @@ -214,7 +227,7 @@ func TestRemote_applyWithParallelism(t *testing.T) { t.Fatal("expected apply operation to fail") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "parallelism values are currently not supported") { t.Fatalf("expected a parallelism error, got: %v", errOutput) } @@ -224,7 +237,7 @@ func TestRemote_applyWithPlan(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationApply(t, "./testdata/apply") + op, configCleanup, playback := testOperationApplyWithDiagnostics(t, "./testdata/apply") defer configCleanup() op.PlanFile = &planfile.Reader{} @@ -243,7 +256,7 @@ func TestRemote_applyWithPlan(t *testing.T) { t.Fatalf("expected plan to be empty") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "saved plan is currently not supported") { t.Fatalf("expected a saved plan error, got: %v", errOutput) } @@ -253,7 +266,7 @@ func TestRemote_applyWithoutRefresh(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationApply(t, "./testdata/apply") + op, configCleanup, playback := testOperationApplyWithDiagnostics(t, "./testdata/apply") defer configCleanup() op.PlanRefresh = false @@ -269,7 +282,7 @@ func TestRemote_applyWithoutRefresh(t *testing.T) { t.Fatal("expected apply operation to fail") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "refresh is currently not supported") { t.Fatalf("expected a refresh error, got: %v", errOutput) } @@ -317,7 +330,7 @@ func TestRemote_applyWithTargetIncompatibleAPIVersion(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationPlan(t, "./testdata/plan") + op, configCleanup, playback := testOperationPlanWithDiagnostics(t, "./testdata/plan") defer configCleanup() // Set the tfe client's RemoteAPIVersion to an empty string, to mimic @@ -342,7 +355,7 @@ func TestRemote_applyWithTargetIncompatibleAPIVersion(t *testing.T) { t.Fatalf("expected plan to be empty") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "Resource targeting is not supported") { t.Fatalf("expected a targeting error, got: %v", errOutput) } @@ -352,7 +365,7 @@ func TestRemote_applyWithVariables(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationApply(t, "./testdata/apply-variables") + op, configCleanup, playback := testOperationApplyWithDiagnostics(t, "./testdata/apply-variables") defer configCleanup() op.Variables = testVariables(terraform.ValueFromNamedFile, "foo", "bar") @@ -368,7 +381,7 @@ func TestRemote_applyWithVariables(t *testing.T) { t.Fatal("expected apply operation to fail") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "variables are currently not supported") { t.Fatalf("expected a variables error, got: %v", errOutput) } @@ -378,7 +391,7 @@ func TestRemote_applyNoConfig(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationApply(t, "./testdata/empty") + op, configCleanup, playback := testOperationApplyWithDiagnostics(t, "./testdata/empty") defer configCleanup() op.Workspace = backend.DefaultStateName @@ -396,7 +409,7 @@ func TestRemote_applyNoConfig(t *testing.T) { t.Fatalf("expected plan to be empty") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "configuration files found") { t.Fatalf("expected configuration files error, got: %v", errOutput) } @@ -443,7 +456,7 @@ func TestRemote_applyNoApprove(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationApply(t, "./testdata/apply") + op, configCleanup, playback := testOperationApplyWithDiagnostics(t, "./testdata/apply") defer configCleanup() input := testInput(t, map[string]string{ @@ -471,7 +484,7 @@ func TestRemote_applyNoApprove(t *testing.T) { t.Fatalf("expected no unused answers, got: %v", input.answers) } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "Apply discarded") { t.Fatalf("expected an apply discarded error, got: %v", errOutput) } @@ -1042,7 +1055,7 @@ func TestRemote_applyPolicyHardFail(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationApply(t, "./testdata/apply-policy-hard-failed") + op, configCleanup, playback := testOperationApplyWithDiagnostics(t, "./testdata/apply-policy-hard-failed") defer configCleanup() input := testInput(t, map[string]string{ @@ -1070,7 +1083,7 @@ func TestRemote_applyPolicyHardFail(t *testing.T) { t.Fatalf("expected an unused answers, got: %v", input.answers) } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "hard failed") { t.Fatalf("expected a policy check error, got: %v", errOutput) } @@ -1142,7 +1155,7 @@ func TestRemote_applyPolicySoftFailAutoApprove(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationApply(t, "./testdata/apply-policy-soft-failed") + op, configCleanup, playback := testOperationApplyWithDiagnostics(t, "./testdata/apply-policy-soft-failed") defer configCleanup() input := testInput(t, map[string]string{ @@ -1171,7 +1184,7 @@ func TestRemote_applyPolicySoftFailAutoApprove(t *testing.T) { t.Fatalf("expected an unused answers, got: %v", input.answers) } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "soft failed") { t.Fatalf("expected a policy check error, got: %v", errOutput) } @@ -1359,7 +1372,7 @@ func TestRemote_applyVersionCheck(t *testing.T) { } // RUN: prepare the apply operation and run it - op, configCleanup := testOperationApply(t, "./testdata/apply") + op, configCleanup, playback := testOperationApplyWithDiagnostics(t, "./testdata/apply") defer configCleanup() input := testInput(t, map[string]string{ @@ -1384,7 +1397,7 @@ func TestRemote_applyVersionCheck(t *testing.T) { if run.Result != backend.OperationFailure { t.Fatalf("expected run to fail, but result was %#v", run.Result) } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, tc.wantErr) { t.Fatalf("missing error %q\noutput: %s", tc.wantErr, errOutput) } diff --git a/backend/remote/backend_plan_test.go b/backend/remote/backend_plan_test.go index a2fecf2d5..f93972770 100644 --- a/backend/remote/backend_plan_test.go +++ b/backend/remote/backend_plan_test.go @@ -17,6 +17,7 @@ import ( "github.com/hashicorp/terraform/plans/planfile" "github.com/hashicorp/terraform/states/statemgr" "github.com/hashicorp/terraform/terraform" + "github.com/hashicorp/terraform/tfdiags" "github.com/mitchellh/cli" ) @@ -26,14 +27,26 @@ func testOperationPlan(t *testing.T, configDir string) (*backend.Operation, func _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) return &backend.Operation{ - ConfigDir: configDir, - ConfigLoader: configLoader, - Parallelism: defaultParallelism, - PlanRefresh: true, - Type: backend.OperationTypePlan, + ConfigDir: configDir, + ConfigLoader: configLoader, + Parallelism: defaultParallelism, + PlanRefresh: true, + ShowDiagnostics: testLogDiagnostics(t), + Type: backend.OperationTypePlan, }, configCleanup } +func testOperationPlanWithDiagnostics(t *testing.T, configDir string) (*backend.Operation, func(), func() tfdiags.Diagnostics) { + t.Helper() + + op, cleanup := testOperationPlan(t, configDir) + + record, playback := testRecordDiagnostics(t) + op.ShowDiagnostics = record + + return op, cleanup, playback +} + func TestRemote_planBasic(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() @@ -148,7 +161,7 @@ func TestRemote_planWithoutPermissions(t *testing.T) { } w.Permissions.CanQueueRun = false - op, configCleanup := testOperationPlan(t, "./testdata/plan") + op, configCleanup, playback := testOperationPlanWithDiagnostics(t, "./testdata/plan") defer configCleanup() op.Workspace = "prod" @@ -163,7 +176,7 @@ func TestRemote_planWithoutPermissions(t *testing.T) { t.Fatal("expected plan operation to fail") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "Insufficient rights to generate a plan") { t.Fatalf("expected a permissions error, got: %v", errOutput) } @@ -173,7 +186,7 @@ func TestRemote_planWithParallelism(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationPlan(t, "./testdata/plan") + op, configCleanup, playback := testOperationPlanWithDiagnostics(t, "./testdata/plan") defer configCleanup() op.Parallelism = 3 @@ -189,7 +202,7 @@ func TestRemote_planWithParallelism(t *testing.T) { t.Fatal("expected plan operation to fail") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "parallelism values are currently not supported") { t.Fatalf("expected a parallelism error, got: %v", errOutput) } @@ -199,7 +212,7 @@ func TestRemote_planWithPlan(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationPlan(t, "./testdata/plan") + op, configCleanup, playback := testOperationPlanWithDiagnostics(t, "./testdata/plan") defer configCleanup() op.PlanFile = &planfile.Reader{} @@ -218,7 +231,7 @@ func TestRemote_planWithPlan(t *testing.T) { t.Fatalf("expected plan to be empty") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "saved plan is currently not supported") { t.Fatalf("expected a saved plan error, got: %v", errOutput) } @@ -228,7 +241,7 @@ func TestRemote_planWithPath(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationPlan(t, "./testdata/plan") + op, configCleanup, playback := testOperationPlanWithDiagnostics(t, "./testdata/plan") defer configCleanup() op.PlanOutPath = "./testdata/plan" @@ -247,7 +260,7 @@ func TestRemote_planWithPath(t *testing.T) { t.Fatalf("expected plan to be empty") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "generated plan is currently not supported") { t.Fatalf("expected a generated plan error, got: %v", errOutput) } @@ -257,7 +270,7 @@ func TestRemote_planWithoutRefresh(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationPlan(t, "./testdata/plan") + op, configCleanup, playback := testOperationPlanWithDiagnostics(t, "./testdata/plan") defer configCleanup() op.PlanRefresh = false @@ -273,7 +286,7 @@ func TestRemote_planWithoutRefresh(t *testing.T) { t.Fatal("expected plan operation to fail") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "refresh is currently not supported") { t.Fatalf("expected a refresh error, got: %v", errOutput) } @@ -355,7 +368,7 @@ func TestRemote_planWithTargetIncompatibleAPIVersion(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationPlan(t, "./testdata/plan") + op, configCleanup, playback := testOperationPlanWithDiagnostics(t, "./testdata/plan") defer configCleanup() // Set the tfe client's RemoteAPIVersion to an empty string, to mimic @@ -380,7 +393,7 @@ func TestRemote_planWithTargetIncompatibleAPIVersion(t *testing.T) { t.Fatalf("expected plan to be empty") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "Resource targeting is not supported") { t.Fatalf("expected a targeting error, got: %v", errOutput) } @@ -390,7 +403,7 @@ func TestRemote_planWithVariables(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationPlan(t, "./testdata/plan-variables") + op, configCleanup, playback := testOperationPlanWithDiagnostics(t, "./testdata/plan-variables") defer configCleanup() op.Variables = testVariables(terraform.ValueFromCLIArg, "foo", "bar") @@ -406,7 +419,7 @@ func TestRemote_planWithVariables(t *testing.T) { t.Fatal("expected plan operation to fail") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "variables are currently not supported") { t.Fatalf("expected a variables error, got: %v", errOutput) } @@ -416,7 +429,7 @@ func TestRemote_planNoConfig(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationPlan(t, "./testdata/empty") + op, configCleanup, playback := testOperationPlanWithDiagnostics(t, "./testdata/empty") defer configCleanup() op.Workspace = backend.DefaultStateName @@ -434,7 +447,7 @@ func TestRemote_planNoConfig(t *testing.T) { t.Fatalf("expected plan to be empty") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "configuration files found") { t.Fatalf("expected configuration files error, got: %v", errOutput) } @@ -875,7 +888,7 @@ func TestRemote_planPolicyHardFail(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationPlan(t, "./testdata/plan-policy-hard-failed") + op, configCleanup, playback := testOperationPlanWithDiagnostics(t, "./testdata/plan-policy-hard-failed") defer configCleanup() op.Workspace = backend.DefaultStateName @@ -893,7 +906,7 @@ func TestRemote_planPolicyHardFail(t *testing.T) { t.Fatalf("expected plan to be empty") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "hard failed") { t.Fatalf("expected a policy check error, got: %v", errOutput) } @@ -914,7 +927,7 @@ func TestRemote_planPolicySoftFail(t *testing.T) { b, bCleanup := testBackendDefault(t) defer bCleanup() - op, configCleanup := testOperationPlan(t, "./testdata/plan-policy-soft-failed") + op, configCleanup, playback := testOperationPlanWithDiagnostics(t, "./testdata/plan-policy-soft-failed") defer configCleanup() op.Workspace = backend.DefaultStateName @@ -932,7 +945,7 @@ func TestRemote_planPolicySoftFail(t *testing.T) { t.Fatalf("expected plan to be empty") } - errOutput := b.CLI.(*cli.MockUi).ErrorWriter.String() + errOutput := playback().Err().Error() if !strings.Contains(errOutput, "soft failed") { t.Fatalf("expected a policy check error, got: %v", errOutput) } diff --git a/backend/remote/cli.go b/backend/remote/cli.go index 5a6afa7ec..9a4f24d08 100644 --- a/backend/remote/cli.go +++ b/backend/remote/cli.go @@ -14,7 +14,6 @@ func (b *Remote) CLIInit(opts *backend.CLIOpts) error { b.CLI = opts.CLI b.CLIColor = opts.CLIColor - b.ShowDiagnostics = opts.ShowDiagnostics b.ContextOpts = opts.ContextOpts return nil diff --git a/backend/remote/testing.go b/backend/remote/testing.go index 07ea95358..249ddcf2b 100644 --- a/backend/remote/testing.go +++ b/backend/remote/testing.go @@ -126,13 +126,6 @@ func testBackend(t *testing.T, obj cty.Value) (*Remote, func()) { b.client.Variables = mc.Variables b.client.Workspaces = mc.Workspaces - b.ShowDiagnostics = func(vals ...interface{}) { - var diags tfdiags.Diagnostics - for _, diag := range diags.Append(vals...) { - b.CLI.Error(diag.Description().Summary) - } - } - // Set local to a local test backend. b.local = testLocalBackend(t, b) @@ -163,7 +156,6 @@ func testLocalBackend(t *testing.T, remote *Remote) backend.Enhanced { b := backendLocal.NewWithBackend(remote) b.CLI = remote.CLI - b.ShowDiagnostics = remote.ShowDiagnostics // Add a test provider to the local backend. p := backendLocal.TestLocalProvider(t, b, "null", &terraform.ProviderSchema{ @@ -307,3 +299,43 @@ func testVariables(s terraform.ValueSourceType, vs ...string) map[string]backend } return vars } + +// testRecordDiagnostics allows tests to record and later inspect diagnostics +// emitted during an Operation. It returns a record function which can be set +// as the ShowDiagnostics value of an Operation, and a playback function which +// returns the recorded diagnostics for inspection. +func testRecordDiagnostics(t *testing.T) (record func(vals ...interface{}), playback func() tfdiags.Diagnostics) { + var diags tfdiags.Diagnostics + record = func(vals ...interface{}) { + diags = diags.Append(vals...) + } + playback = func() tfdiags.Diagnostics { + diags.Sort() + return diags + } + return +} + +// testLogDiagnostics returns a function which can be used as the +// ShowDiagnostics value for an Operation, in order to help debugging during +// tests. Any calls to this function result in test logs. +func testLogDiagnostics(t *testing.T) func(vals ...interface{}) { + return func(vals ...interface{}) { + var diags tfdiags.Diagnostics + diags = diags.Append(vals...) + diags.Sort() + + for _, diag := range diags { + // NOTE: Since the caller here is not directly the TestLocal + // function, t.Helper doesn't apply and so the log source + // isn't correctly shown in the test log output. This seems + // unavoidable as long as this is happening so indirectly. + desc := diag.Description() + if desc.Detail != "" { + t.Logf("%s: %s", desc.Summary, desc.Detail) + } else { + t.Log(desc.Summary) + } + } + } +} diff --git a/command/apply.go b/command/apply.go index 0b4df0af6..96ced305c 100644 --- a/command/apply.go +++ b/command/apply.go @@ -173,6 +173,7 @@ func (c *ApplyCommand) Run(args []string) int { opReq.Destroy = c.Destroy opReq.PlanFile = planFile opReq.PlanRefresh = refresh + opReq.ShowDiagnostics = c.showDiagnostics opReq.Type = backend.OperationTypeApply opReq.ConfigLoader, err = c.initConfigLoader() diff --git a/command/meta_backend.go b/command/meta_backend.go index f7f2cba84..2e01a428f 100644 --- a/command/meta_backend.go +++ b/command/meta_backend.go @@ -309,7 +309,6 @@ func (m *Meta) backendCLIOpts() (*backend.CLIOpts, error) { CLI: m.Ui, CLIColor: m.Colorize(), Streams: m.Streams, - ShowDiagnostics: m.showDiagnostics, StatePath: m.statePath, StateOutPath: m.stateOutPath, StateBackupPath: m.backupPath, diff --git a/command/plan.go b/command/plan.go index ce5ea0800..38e2b77d0 100644 --- a/command/plan.go +++ b/command/plan.go @@ -84,6 +84,7 @@ func (c *PlanCommand) Run(args []string) int { opReq.Destroy = destroy opReq.PlanOutPath = outPath opReq.PlanRefresh = refresh + opReq.ShowDiagnostics = c.showDiagnostics opReq.Type = backend.OperationTypePlan opReq.ConfigLoader, err = c.initConfigLoader() diff --git a/command/refresh.go b/command/refresh.go index 91dd28ce1..00b4d9d77 100644 --- a/command/refresh.go +++ b/command/refresh.go @@ -75,6 +75,7 @@ func (c *RefreshCommand) Run(args []string) int { // Build the operation opReq := c.Operation(b) opReq.ConfigDir = configPath + opReq.ShowDiagnostics = c.showDiagnostics opReq.Type = backend.OperationTypeRefresh opReq.ConfigLoader, err = c.initConfigLoader()