From 604e65bb6234d8e90db5d1228328332a3ed9985c Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Tue, 30 Jun 2020 14:05:29 -0400 Subject: [PATCH] Revert "backend/local: release lock if there is an error in Context() (#25427)" This reverts commit 1ba0d615e738aa447715cca197619dd54988ea3b. --- backend/local/backend_local.go | 12 ----- backend/local/backend_local_test.go | 64 -------------------------- backend/remote/backend_context_test.go | 13 ------ command/console.go | 11 +++-- 4 files changed, 6 insertions(+), 94 deletions(-) delete mode 100644 backend/local/backend_local_test.go diff --git a/backend/local/backend_local.go b/backend/local/backend_local.go index b06de8016..4fffc5b66 100644 --- a/backend/local/backend_local.go +++ b/backend/local/backend_local.go @@ -49,18 +49,6 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, *configload. diags = diags.Append(errwrap.Wrapf("Error locking state: {{err}}", err)) return nil, nil, nil, diags } - - defer func() { - // If we're returning with errors, and thus not producing a valid - // context, we'll want to avoid leaving the workspace locked. - if diags.HasErrors() { - err := op.StateLocker.Unlock(nil) - if err != nil { - diags = diags.Append(errwrap.Wrapf("Error unlocking state: {{err}}", err)) - } - } - }() - log.Printf("[TRACE] backend/local: reading remote state for workspace %q", op.Workspace) if err := s.RefreshState(); err != nil { diags = diags.Append(errwrap.Wrapf("Error loading state: {{err}}", err)) diff --git a/backend/local/backend_local_test.go b/backend/local/backend_local_test.go deleted file mode 100644 index f8470fbba..000000000 --- a/backend/local/backend_local_test.go +++ /dev/null @@ -1,64 +0,0 @@ -package local - -import ( - "testing" - - "github.com/hashicorp/terraform/backend" - "github.com/hashicorp/terraform/internal/initwd" - "github.com/hashicorp/terraform/states/statemgr" -) - -func TestLocalContext(t *testing.T) { - configDir := "./testdata/empty" - b, cleanup := TestLocal(t) - defer cleanup() - - _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) - defer configCleanup() - - op := &backend.Operation{ - ConfigDir: configDir, - ConfigLoader: configLoader, - Workspace: backend.DefaultStateName, - LockState: true, - } - - _, _, diags := b.Context(op) - if diags.HasErrors() { - t.Fatalf("unexpected error: %s", diags.Err().Error()) - } - - // Conext() retains a lock on success, so this should fail. - stateMgr, _ := b.StateMgr(backend.DefaultStateName) - if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err == nil { - t.Fatalf("unexpected success locking state") - } -} - -func TestLocalContext_error(t *testing.T) { - configDir := "./testdata/apply-error" - b, cleanup := TestLocal(t) - defer cleanup() - - _, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir) - defer configCleanup() - - op := &backend.Operation{ - ConfigDir: configDir, - ConfigLoader: configLoader, - Workspace: backend.DefaultStateName, - LockState: true, - } - - _, _, diags := b.Context(op) - if !diags.HasErrors() { - t.Fatal("unexpected success") - } - - // When Context() returns an error, it also unlocks the state. - // This should therefore succeed. - stateMgr, _ := b.StateMgr(backend.DefaultStateName) - if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil { - t.Fatalf("unexpected error locking state: %s", err.Error()) - } -} diff --git a/backend/remote/backend_context_test.go b/backend/remote/backend_context_test.go index 50f141354..a7fa5e713 100644 --- a/backend/remote/backend_context_test.go +++ b/backend/remote/backend_context_test.go @@ -8,7 +8,6 @@ import ( "github.com/hashicorp/terraform/backend" "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/internal/initwd" - "github.com/hashicorp/terraform/states/statemgr" "github.com/zclconf/go-cty/cty" ) @@ -187,7 +186,6 @@ func TestRemoteContextWithVars(t *testing.T) { ConfigDir: configDir, ConfigLoader: configLoader, Workspace: backend.DefaultStateName, - LockState: true, } v := test.Opts @@ -207,21 +205,10 @@ func TestRemoteContextWithVars(t *testing.T) { if errStr != test.WantError { t.Fatalf("wrong error\ngot: %s\nwant: %s", errStr, test.WantError) } - // When Context() returns an error, it should unlock the state, - // so re-locking it is expected to succeed. - stateMgr, _ := b.StateMgr(backend.DefaultStateName) - if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil { - t.Fatalf("unexpected error locking state: %s", err.Error()) - } } else { if diags.HasErrors() { t.Fatalf("unexpected error\ngot: %s\nwant: ", diags.Err().Error()) } - // If Context() succeeded, this should fail w/ "workspace already locked" - stateMgr, _ := b.StateMgr(backend.DefaultStateName) - if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err == nil { - t.Fatal("unexpected success locking state after Context") - } } }) } diff --git a/command/console.go b/command/console.go index be317b426..1fa2cb875 100644 --- a/command/console.go +++ b/command/console.go @@ -92,11 +92,6 @@ func (c *ConsoleCommand) Run(args []string) int { // Get the context ctx, _, ctxDiags := local.Context(opReq) - diags = diags.Append(ctxDiags) - if ctxDiags.HasErrors() { - c.showDiagnostics(diags) - return 1 - } // Creating the context can result in a lock, so ensure we release it defer func() { @@ -106,6 +101,12 @@ func (c *ConsoleCommand) Run(args []string) int { } }() + diags = diags.Append(ctxDiags) + if ctxDiags.HasErrors() { + c.showDiagnostics(diags) + return 1 + } + // Setup the UI so we can output directly to stdout ui := &cli.BasicUi{ Writer: wrappedstreams.Stdout(),