From b1fdbd7db851eed5bb3e45349145f2844ca9c14e Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Fri, 5 Oct 2018 17:39:06 +0200 Subject: [PATCH] Allow enhanced backends to pass custom exit codes In some cases this is needed to keep the UX clean and to make sure any remote exit codes are passed through to the local process. The most obvious example for this is when using the "remote" backend. This backend runs Terraform remotely and stream the output back to the local terminal. When an error occurs during the remote execution, all the needed error information will already be in the streamed output. So if we then return an error ourselves, users will get the same errors twice. By allowing the backend to specify the correct exit code, the UX remains the same while preserving the correct exit codes. --- backend/backend.go | 4 ++ backend/remote/backend.go | 40 +++++++---- backend/remote/backend_apply_test.go | 71 +++++++++++++++++++ backend/remote/backend_mock.go | 30 +++++--- backend/remote/backend_plan_test.go | 53 ++++++++++++++ .../test-fixtures/apply-with-error/main.tf | 5 ++ .../test-fixtures/apply-with-error/plan.log | 10 +++ .../test-fixtures/plan-with-error/main.tf | 5 ++ .../test-fixtures/plan-with-error/plan.log | 10 +++ command/apply.go | 2 +- command/plan.go | 2 +- 11 files changed, 205 insertions(+), 27 deletions(-) create mode 100644 backend/remote/test-fixtures/apply-with-error/main.tf create mode 100644 backend/remote/test-fixtures/apply-with-error/plan.log create mode 100644 backend/remote/test-fixtures/plan-with-error/main.tf create mode 100644 backend/remote/test-fixtures/plan-with-error/plan.log diff --git a/backend/backend.go b/backend/backend.go index 7f9435e99..840cb06ac 100644 --- a/backend/backend.go +++ b/backend/backend.go @@ -188,6 +188,10 @@ type RunningOperation struct { // the operation has completed. Err error + // ExitCode can be used to set a custom exit code. This enables enhanced + // backends to set specific exit codes that miror any remote exit codes. + ExitCode int + // PlanEmpty is populated after a Plan operation completes without error // to note whether a plan is empty or has changes. PlanEmpty bool diff --git a/backend/remote/backend.go b/backend/remote/backend.go index 8730904be..2dcbc8b0d 100644 --- a/backend/remote/backend.go +++ b/backend/remote/backend.go @@ -416,7 +416,8 @@ func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend // the runninCtx is only used to block until the operation returns. runningCtx, done := context.WithCancel(context.Background()) runningOp := &backend.RunningOperation{ - Context: runningCtx, + Context: runningCtx, + PlanEmpty: true, } // stopCtx wraps the context passed in, and is used to signal a graceful Stop. @@ -436,13 +437,30 @@ func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend defer b.opLock.Unlock() - r, err := f(stopCtx, cancelCtx, op) - if err != nil && err != context.Canceled { - runningOp.Err = err + r, opErr := f(stopCtx, cancelCtx, op) + if opErr != nil && opErr != context.Canceled { + runningOp.Err = opErr + return } - if r != nil && err == context.Canceled { - runningOp.Err = b.cancel(cancelCtx, op, r.ID) + if r != nil { + // Retrieve the run to get its current status. + r, err := b.client.Runs.Read(cancelCtx, r.ID) + if err != nil { + runningOp.Err = generalError("error retrieving run", err) + return + } + + // Record if there are any changes. + runningOp.PlanEmpty = !r.HasChanges + + if opErr == context.Canceled { + runningOp.Err = b.cancel(cancelCtx, op, r) + } + + if runningOp.Err == nil && r.Status == tfe.RunErrored { + runningOp.ExitCode = 1 + } } }() @@ -450,13 +468,7 @@ func (b *Remote) Operation(ctx context.Context, op *backend.Operation) (*backend return runningOp, nil } -func (b *Remote) cancel(cancelCtx context.Context, op *backend.Operation, runID string) error { - // Retrieve the run to get its current status. - r, err := b.client.Runs.Read(cancelCtx, runID) - if err != nil { - return generalError("error cancelling run", err) - } - +func (b *Remote) cancel(cancelCtx context.Context, op *backend.Operation, r *tfe.Run) error { if r.Status == tfe.RunPending && r.Actions.IsCancelable { // Only ask if the remote operation should be canceled // if the auto approve flag is not set. @@ -483,7 +495,7 @@ func (b *Remote) cancel(cancelCtx context.Context, op *backend.Operation, runID } // Try to cancel the remote operation. - err = b.client.Runs.Cancel(cancelCtx, r.ID, tfe.RunCancelOptions{}) + err := b.client.Runs.Cancel(cancelCtx, r.ID, tfe.RunCancelOptions{}) if err != nil { return generalError("error cancelling run", err) } diff --git a/backend/remote/backend_apply_test.go b/backend/remote/backend_apply_test.go index 2dbf00fdd..d2794b795 100644 --- a/backend/remote/backend_apply_test.go +++ b/backend/remote/backend_apply_test.go @@ -49,6 +49,9 @@ func TestRemote_applyBasic(t *testing.T) { if run.Err != nil { t.Fatalf("error running operation: %v", run.Err) } + if run.PlanEmpty { + t.Fatalf("expected a non-empty plan") + } if len(input.answers) > 0 { t.Fatalf("expected no unused answers, got: %v", input.answers) @@ -132,6 +135,9 @@ func TestRemote_applyWithVCS(t *testing.T) { if run.Err == nil { t.Fatalf("expected an apply error, got: %v", run.Err) } + if !run.PlanEmpty { + t.Fatalf("expected plan to be empty") + } if !strings.Contains(run.Err.Error(), "not allowed for workspaces with a VCS") { t.Fatalf("expected a VCS error, got: %v", run.Err) } @@ -182,6 +188,9 @@ func TestRemote_applyWithPlan(t *testing.T) { if run.Err == nil { t.Fatalf("expected an apply error, got: %v", run.Err) } + if !run.PlanEmpty { + t.Fatalf("expected plan to be empty") + } if !strings.Contains(run.Err.Error(), "saved plan is currently not supported") { t.Fatalf("expected a saved plan error, got: %v", run.Err) } @@ -232,6 +241,9 @@ func TestRemote_applyWithTarget(t *testing.T) { if run.Err == nil { t.Fatalf("expected an apply error, got: %v", run.Err) } + if !run.PlanEmpty { + t.Fatalf("expected plan to be empty") + } if !strings.Contains(run.Err.Error(), "targeting is currently not supported") { t.Fatalf("expected a targeting error, got: %v", run.Err) } @@ -278,6 +290,9 @@ func TestRemote_applyNoConfig(t *testing.T) { if run.Err == nil { t.Fatalf("expected an apply error, got: %v", run.Err) } + if !run.PlanEmpty { + t.Fatalf("expected plan to be empty") + } if !strings.Contains(run.Err.Error(), "configuration files found") { t.Fatalf("expected configuration files error, got: %v", run.Err) } @@ -302,6 +317,9 @@ func TestRemote_applyNoChanges(t *testing.T) { if run.Err != nil { t.Fatalf("error running operation: %v", run.Err) } + if !run.PlanEmpty { + t.Fatalf("expected plan to be empty") + } output := b.CLI.(*cli.MockUi).OutputWriter.String() if !strings.Contains(output, "No changes. Infrastructure is up-to-date.") { @@ -334,6 +352,9 @@ func TestRemote_applyNoApprove(t *testing.T) { if run.Err == nil { t.Fatalf("expected an apply error, got: %v", run.Err) } + if !run.PlanEmpty { + t.Fatalf("expected plan to be empty") + } if !strings.Contains(run.Err.Error(), "Apply discarded") { t.Fatalf("expected an apply discarded error, got: %v", run.Err) } @@ -368,6 +389,9 @@ func TestRemote_applyAutoApprove(t *testing.T) { if run.Err != nil { t.Fatalf("error running operation: %v", run.Err) } + if run.PlanEmpty { + t.Fatalf("expected a non-empty plan") + } if len(input.answers) != 1 { t.Fatalf("expected an unused answer, got: %v", input.answers) @@ -479,6 +503,9 @@ func TestRemote_applyDestroy(t *testing.T) { if run.Err != nil { t.Fatalf("error running operation: %v", run.Err) } + if run.PlanEmpty { + t.Fatalf("expected a non-empty plan") + } if len(input.answers) > 0 { t.Fatalf("expected no unused answers, got: %v", input.answers) @@ -516,6 +543,9 @@ func TestRemote_applyDestroyNoConfig(t *testing.T) { if run.Err != nil { t.Fatalf("unexpected apply error: %v", run.Err) } + if run.PlanEmpty { + t.Fatalf("expected a non-empty plan") + } if len(input.answers) > 0 { t.Fatalf("expected no unused answers, got: %v", input.answers) @@ -547,6 +577,9 @@ func TestRemote_applyPolicyPass(t *testing.T) { if run.Err != nil { t.Fatalf("error running operation: %v", run.Err) } + if run.PlanEmpty { + t.Fatalf("expected a non-empty plan") + } if len(input.answers) > 0 { t.Fatalf("expected no unused answers, got: %v", input.answers) @@ -589,6 +622,9 @@ func TestRemote_applyPolicyHardFail(t *testing.T) { if run.Err == nil { t.Fatalf("expected an apply error, got: %v", run.Err) } + if !run.PlanEmpty { + t.Fatalf("expected plan to be empty") + } if !strings.Contains(run.Err.Error(), "hard failed") { t.Fatalf("expected a policy check error, got: %v", run.Err) } @@ -634,6 +670,9 @@ func TestRemote_applyPolicySoftFail(t *testing.T) { if run.Err != nil { t.Fatalf("error running operation: %v", run.Err) } + if run.PlanEmpty { + t.Fatalf("expected a non-empty plan") + } if len(input.answers) > 0 { t.Fatalf("expected no unused answers, got: %v", input.answers) @@ -677,6 +716,9 @@ func TestRemote_applyPolicySoftFailAutoApprove(t *testing.T) { if run.Err != nil { t.Fatalf("error running operation: %v", run.Err) } + if run.PlanEmpty { + t.Fatalf("expected a non-empty plan") + } if len(input.answers) > 0 { t.Fatalf("expected no unused answers, got: %v", input.answers) @@ -693,3 +735,32 @@ func TestRemote_applyPolicySoftFailAutoApprove(t *testing.T) { t.Fatalf("missing apply summery in output: %s", output) } } + +func TestRemote_applyWithRemoteError(t *testing.T) { + b := testBackendDefault(t) + + mod, modCleanup := module.TestTree(t, "./test-fixtures/apply-with-error") + defer modCleanup() + + op := testOperationApply() + op.Module = mod + op.Workspace = backend.DefaultStateName + + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("error starting operation: %v", err) + } + + <-run.Done() + if run.Err != nil { + t.Fatalf("error running operation: %v", run.Err) + } + if run.ExitCode != 1 { + t.Fatalf("expected exit code 1, got %d", run.ExitCode) + } + + output := b.CLI.(*cli.MockUi).OutputWriter.String() + if !strings.Contains(output, "null_resource.foo: 1 error") { + t.Fatalf("missing apply error in output: %s", output) + } +} diff --git a/backend/remote/backend_mock.go b/backend/remote/backend_mock.go index 93fd2558e..909dcb947 100644 --- a/backend/remote/backend_mock.go +++ b/backend/remote/backend_mock.go @@ -609,9 +609,9 @@ func (m *mockRuns) Create(ctx context.Context, options tfe.RunCreateOptions) (*t r := &tfe.Run{ ID: generateID("run-"), - Actions: &tfe.RunActions{}, + Actions: &tfe.RunActions{IsCancelable: true}, Apply: a, - HasChanges: true, + HasChanges: false, Permissions: &tfe.RunPermissions{}, Plan: p, Status: tfe.RunPending, @@ -625,14 +625,6 @@ func (m *mockRuns) Create(ctx context.Context, options tfe.RunCreateOptions) (*t r.IsDestroy = *options.IsDestroy } - logs, _ := ioutil.ReadFile(m.client.Plans.logs[p.LogReadURL]) - if r.IsDestroy || !bytes.Contains(logs, []byte("No changes. Infrastructure is up-to-date.")) { - r.Actions.IsCancelable = true - r.Actions.IsConfirmable = true - r.HasChanges = true - r.Permissions.CanApply = true - } - m.runs[r.ID] = r m.workspaces[options.Workspace.ID] = append(m.workspaces[options.Workspace.ID], r) @@ -653,12 +645,28 @@ func (m *mockRuns) Read(ctx context.Context, runID string) (*tfe.Run, error) { } } - if !pending { + if !pending && r.Status == tfe.RunPending { // Only update the status if there are no other pending runs. r.Status = tfe.RunPlanning r.Plan.Status = tfe.PlanRunning } + logs, _ := ioutil.ReadFile(m.client.Plans.logs[r.Plan.LogReadURL]) + if r.Plan.Status == tfe.PlanFinished { + if r.IsDestroy || bytes.Contains(logs, []byte("1 to add, 0 to change, 0 to destroy")) { + r.Actions.IsCancelable = false + r.Actions.IsConfirmable = true + r.HasChanges = true + r.Permissions.CanApply = true + } + + if bytes.Contains(logs, []byte("null_resource.foo: 1 error")) { + r.Actions.IsCancelable = false + r.HasChanges = false + r.Status = tfe.RunErrored + } + } + return r, nil } diff --git a/backend/remote/backend_plan_test.go b/backend/remote/backend_plan_test.go index 68cd6431b..a9ac9a5d5 100644 --- a/backend/remote/backend_plan_test.go +++ b/backend/remote/backend_plan_test.go @@ -44,6 +44,9 @@ func TestRemote_planBasic(t *testing.T) { if run.Err != nil { t.Fatalf("error running operation: %v", run.Err) } + if run.PlanEmpty { + t.Fatal("expected a non-empty plan") + } output := b.CLI.(*cli.MockUi).OutputWriter.String() if !strings.Contains(output, "1 to add, 0 to change, 0 to destroy") { @@ -158,6 +161,9 @@ func TestRemote_planWithPlan(t *testing.T) { if run.Err == nil { t.Fatalf("expected a plan error, got: %v", run.Err) } + if !run.PlanEmpty { + t.Fatalf("expected plan to be empty") + } if !strings.Contains(run.Err.Error(), "saved plan is currently not supported") { t.Fatalf("expected a saved plan error, got: %v", run.Err) } @@ -183,6 +189,9 @@ func TestRemote_planWithPath(t *testing.T) { if run.Err == nil { t.Fatalf("expected a plan error, got: %v", run.Err) } + if !run.PlanEmpty { + t.Fatalf("expected plan to be empty") + } if !strings.Contains(run.Err.Error(), "generated plan is currently not supported") { t.Fatalf("expected a generated plan error, got: %v", run.Err) } @@ -233,6 +242,9 @@ func TestRemote_planWithTarget(t *testing.T) { if run.Err == nil { t.Fatalf("expected a plan error, got: %v", run.Err) } + if !run.PlanEmpty { + t.Fatalf("expected plan to be empty") + } if !strings.Contains(run.Err.Error(), "targeting is currently not supported") { t.Fatalf("expected a targeting error, got: %v", run.Err) } @@ -279,6 +291,9 @@ func TestRemote_planNoConfig(t *testing.T) { if run.Err == nil { t.Fatalf("expected a plan error, got: %v", run.Err) } + if !run.PlanEmpty { + t.Fatalf("expected plan to be empty") + } if !strings.Contains(run.Err.Error(), "configuration files found") { t.Fatalf("expected configuration files error, got: %v", run.Err) } @@ -372,6 +387,9 @@ func TestRemote_planDestroy(t *testing.T) { if run.Err != nil { t.Fatalf("unexpected plan error: %v", run.Err) } + if run.PlanEmpty { + t.Fatalf("expected a non-empty plan") + } } func TestRemote_planDestroyNoConfig(t *testing.T) { @@ -391,6 +409,9 @@ func TestRemote_planDestroyNoConfig(t *testing.T) { if run.Err != nil { t.Fatalf("unexpected plan error: %v", run.Err) } + if run.PlanEmpty { + t.Fatalf("expected a non-empty plan") + } } func TestRemote_planWithWorkingDirectory(t *testing.T) { @@ -422,9 +443,41 @@ func TestRemote_planWithWorkingDirectory(t *testing.T) { if run.Err != nil { t.Fatalf("error running operation: %v", run.Err) } + if run.PlanEmpty { + t.Fatalf("expected a non-empty plan") + } output := b.CLI.(*cli.MockUi).OutputWriter.String() if !strings.Contains(output, "1 to add, 0 to change, 0 to destroy") { t.Fatalf("missing plan summery in output: %s", output) } } + +func TestRemote_planWithRemoteError(t *testing.T) { + b := testBackendDefault(t) + + mod, modCleanup := module.TestTree(t, "./test-fixtures/plan-with-error") + defer modCleanup() + + op := testOperationPlan() + op.Module = mod + op.Workspace = backend.DefaultStateName + + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("error starting operation: %v", err) + } + + <-run.Done() + if run.Err != nil { + t.Fatalf("error running operation: %v", run.Err) + } + if run.ExitCode != 1 { + t.Fatalf("expected exit code 1, got %d", run.ExitCode) + } + + output := b.CLI.(*cli.MockUi).OutputWriter.String() + if !strings.Contains(output, "null_resource.foo: 1 error") { + t.Fatalf("missing plan error in output: %s", output) + } +} diff --git a/backend/remote/test-fixtures/apply-with-error/main.tf b/backend/remote/test-fixtures/apply-with-error/main.tf new file mode 100644 index 000000000..bc45f28f5 --- /dev/null +++ b/backend/remote/test-fixtures/apply-with-error/main.tf @@ -0,0 +1,5 @@ +resource "null_resource" "foo" { + triggers { + random = "${guid()}" + } +} diff --git a/backend/remote/test-fixtures/apply-with-error/plan.log b/backend/remote/test-fixtures/apply-with-error/plan.log new file mode 100644 index 000000000..4344a3722 --- /dev/null +++ b/backend/remote/test-fixtures/apply-with-error/plan.log @@ -0,0 +1,10 @@ +Terraform v0.11.7 + +Configuring remote state backend... +Initializing Terraform configuration... + +Error: null_resource.foo: 1 error(s) occurred: + +* null_resource.foo: 1:3: unknown function called: guid in: + +${guid()} diff --git a/backend/remote/test-fixtures/plan-with-error/main.tf b/backend/remote/test-fixtures/plan-with-error/main.tf new file mode 100644 index 000000000..bc45f28f5 --- /dev/null +++ b/backend/remote/test-fixtures/plan-with-error/main.tf @@ -0,0 +1,5 @@ +resource "null_resource" "foo" { + triggers { + random = "${guid()}" + } +} diff --git a/backend/remote/test-fixtures/plan-with-error/plan.log b/backend/remote/test-fixtures/plan-with-error/plan.log new file mode 100644 index 000000000..4344a3722 --- /dev/null +++ b/backend/remote/test-fixtures/plan-with-error/plan.log @@ -0,0 +1,10 @@ +Terraform v0.11.7 + +Configuring remote state backend... +Initializing Terraform configuration... + +Error: null_resource.foo: 1 error(s) occurred: + +* null_resource.foo: 1:3: unknown function called: guid in: + +${guid()} diff --git a/command/apply.go b/command/apply.go index 8c1786619..14baa5aa7 100644 --- a/command/apply.go +++ b/command/apply.go @@ -177,7 +177,7 @@ func (c *ApplyCommand) Run(args []string) int { } } - return 0 + return op.ExitCode } func (c *ApplyCommand) Help() string { diff --git a/command/plan.go b/command/plan.go index 0db56fbe4..06b8d2ca8 100644 --- a/command/plan.go +++ b/command/plan.go @@ -121,7 +121,7 @@ func (c *PlanCommand) Run(args []string) int { return 2 } - return 0 + return op.ExitCode } func (c *PlanCommand) Help() string {