command/state: lock when pushing state

Next to adding the locking for the `state push` command, this commit also fixes a small bug where the lock would not be propertly released when running the `state show` command.

And finally it renames some variables in the `[un]taint` code in order to try to standardize the var names of a few frequently used variables (e.g. statemgr.Full, states.State, states.SyncState).

command/apply_destroy_test.go

@@ -127,7 +127,7 @@ func TestApply_destroyLockedState(t *testing.T) {
 	})
 	statePath := testStateFile(t, originalState)
 
-	unlock, err := testLockState("./testdata", statePath)
+	unlock, err := testLockState(testDataDir, statePath)
 	if err != nil {
 		t.Fatal(err)
 	}

command/apply_test.go

@@ -64,7 +64,7 @@ func TestApply(t *testing.T) {
 func TestApply_lockedState(t *testing.T) {
 	statePath := testTempFile(t)
 
-	unlock, err := testLockState("./testdata", statePath)
+	unlock, err := testLockState(testDataDir, statePath)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -98,7 +98,7 @@ func TestApply_lockedState(t *testing.T) {
 func TestApply_lockedStateWait(t *testing.T) {
 	statePath := testTempFile(t)
 
-	unlock, err := testLockState("./testdata", statePath)
+	unlock, err := testLockState(testDataDir, statePath)
 	if err != nil {
 		t.Fatal(err)
 	}

command/command_test.go

@@ -38,8 +38,11 @@ import (
 	backendInit "github.com/hashicorp/terraform/backend/init"
 )
 
-// This is the directory where our test fixtures are.
+// These are the directories for our test data and fixtures.
-var fixtureDir = "./test-fixtures"
+var (
+	fixtureDir  = "./test-fixtures"
+	testDataDir = "./testdata"
+)
 
 // a top level temp directory which will be cleaned after all tests
 var testingDir string
@@ -50,14 +53,19 @@ func init() {
 	// Initialize the backends
 	backendInit.Init(nil)
 
-	// Expand the fixture dir on init because we change the working
+	// Expand the data and fixture dirs on init because
-	// directory in some tests.
+	// we change the working directory in some tests.
 	var err error
 	fixtureDir, err = filepath.Abs(fixtureDir)
 	if err != nil {
 		panic(err)
 	}
 
+	testDataDir, err = filepath.Abs(testDataDir)
+	if err != nil {
+		panic(err)
+	}
+
 	testingDir, err = ioutil.TempDir(testingDir, "tf")
 	if err != nil {
 		panic(err)
@@ -783,7 +791,7 @@ func testRemoteState(t *testing.T, s *states.State, c int) (*terraform.State, *h
 
 // testlockState calls a separate process to the lock the state file at path.
 // deferFunc should be called in the caller to properly unlock the file.
-// Since many tests change the working durectory, the sourcedir argument must be
+// Since many tests change the working directory, the sourcedir argument must be
 // supplied to locate the statelocker.go source.
 func testLockState(sourceDir, path string) (func(), error) {
 	// build and run the binary ourselves so we can quickly terminate it for cleanup
@@ -798,7 +806,10 @@ func testLockState(sourceDir, path string) (func(), error) {
 	source := filepath.Join(sourceDir, "statelocker.go")
 	lockBin := filepath.Join(buildDir, "statelocker")
 
-	out, err := exec.Command("go", "build", "-mod=vendor", "-o", lockBin, source).CombinedOutput()
+	cmd := exec.Command("go", "build", "-mod=vendor", "-o", lockBin, source)
+	cmd.Dir = filepath.Dir(sourceDir)
+
+	out, err := cmd.CombinedOutput()
 	if err != nil {
 		cleanFunc()
 		return nil, fmt.Errorf("%s %s", err, out)

command/import.go

@@ -235,6 +235,7 @@ func (c *ImportCommand) Run(args []string) int {
 		return 1
 	}
 
+	// Make sure to unlock the state
 	defer func() {
 		err := opReq.StateLocker.Unlock(nil)
 		if err != nil {

command/plan_test.go

@@ -56,7 +56,7 @@ func TestPlan_lockedState(t *testing.T) {
 	}
 
 	testPath := testFixturePath("plan")
-	unlock, err := testLockState("./testdata", filepath.Join(testPath, DefaultStateFilename))
+	unlock, err := testLockState(testDataDir, filepath.Join(testPath, DefaultStateFilename))
 	if err != nil {
 		t.Fatal(err)
 	}

command/refresh_test.go

@@ -115,7 +115,7 @@ func TestRefresh_lockedState(t *testing.T) {
 	state := testState()
 	statePath := testStateFile(t, state)
 
-	unlock, err := testLockState("./testdata", statePath)
+	unlock, err := testLockState(testDataDir, statePath)
 	if err != nil {
 		t.Fatal(err)
 	}

command/state_push.go

@@ -1,11 +1,13 @@
 package command
 
 import (
+	"context"
 	"fmt"
 	"io"
 	"os"
 	"strings"
 
+	"github.com/hashicorp/terraform/command/clistate"
 	"github.com/hashicorp/terraform/states/statefile"
 	"github.com/hashicorp/terraform/states/statemgr"
 	"github.com/mitchellh/cli"
@@ -77,6 +79,16 @@ func (c *StatePushCommand) Run(args []string) int {
 		c.Ui.Error(fmt.Sprintf("Failed to load destination state: %s", err))
 		return 1
 	}
+
+	if c.stateLock {
+		stateLocker := clistate.NewLocker(context.Background(), c.stateLockTimeout, c.Ui, c.Colorize())
+		if err := stateLocker.Lock(stateMgr, "taint"); err != nil {
+			c.Ui.Error(fmt.Sprintf("Error locking state: %s", err))
+			return 1
+		}
+		defer stateLocker.Unlock(nil)
+	}
+
 	if err := stateMgr.RefreshState(); err != nil {
 		c.Ui.Error(fmt.Sprintf("Failed to refresh destination state: %s", err))
 		return 1

command/state_push_test.go

@@ -3,6 +3,7 @@ package command
 import (
 	"bytes"
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/hashicorp/terraform/backend"
@@ -41,6 +42,37 @@ func TestStatePush_empty(t *testing.T) {
 	}
 }
 
+func TestStatePush_lockedState(t *testing.T) {
+	// Create a temporary working directory that is empty
+	td := tempDir(t)
+	copy.CopyDir(testFixturePath("state-push-good"), td)
+	defer os.RemoveAll(td)
+	defer testChdir(t, td)()
+
+	p := testProvider()
+	ui := new(cli.MockUi)
+	c := &StatePushCommand{
+		Meta: Meta{
+			testingOverrides: metaOverridesForProvider(p),
+			Ui:               ui,
+		},
+	}
+
+	unlock, err := testLockState(testDataDir, "local-state.tfstate")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer unlock()
+
+	args := []string{"replace.tfstate"}
+	if code := c.Run(args); code != 1 {
+		t.Fatalf("bad: %d", code)
+	}
+	if !strings.Contains(ui.ErrorWriter.String(), "Error acquiring the state lock") {
+		t.Fatalf("expected a lock error, got: %s", ui.ErrorWriter.String())
+	}
+}
+
 func TestStatePush_replaceMatch(t *testing.T) {
 	// Create a temporary working directory that is empty
 	td := tempDir(t)

command/state_show.go

@@ -79,6 +79,14 @@ func (c *StateShowCommand) Run(args []string) int {
 		return 1
 	}
 
+	// Make sure to unlock the state
+	defer func() {
+		err := opReq.StateLocker.Unlock(nil)
+		if err != nil {
+			c.Ui.Error(err.Error())
+		}
+	}()
+
 	// Get the schemas from the context
 	schemas := ctx.Schemas()
 

command/taint.go

@@ -76,7 +76,7 @@ func (c *TaintCommand) Run(args []string) int {
 
 	// Get the state
 	env := c.Workspace()
-	st, err := b.StateMgr(env)
+	stateMgr, err := b.StateMgr(env)
 	if err != nil {
 		c.Ui.Error(fmt.Sprintf("Failed to load state: %s", err))
 		return 1
@@ -84,21 +84,21 @@ func (c *TaintCommand) Run(args []string) int {
 
 	if c.stateLock {
 		stateLocker := clistate.NewLocker(context.Background(), c.stateLockTimeout, c.Ui, c.Colorize())
-		if err := stateLocker.Lock(st, "taint"); err != nil {
+		if err := stateLocker.Lock(stateMgr, "taint"); err != nil {
 			c.Ui.Error(fmt.Sprintf("Error locking state: %s", err))
 			return 1
 		}
 		defer stateLocker.Unlock(nil)
 	}
 
-	if err := st.RefreshState(); err != nil {
+	if err := stateMgr.RefreshState(); err != nil {
 		c.Ui.Error(fmt.Sprintf("Failed to load state: %s", err))
 		return 1
 	}
 
 	// Get the actual state structure
-	s := st.State()
+	state := stateMgr.State()
-	if s.Empty() {
+	if state.Empty() {
 		if allowMissing {
 			return c.allowMissingExit(addr)
 		}
@@ -112,11 +112,11 @@ func (c *TaintCommand) Run(args []string) int {
 		return 1
 	}
 
-	state := s.SyncWrapper()
+	ss := state.SyncWrapper()
 
 	// Get the resource and instance we're going to taint
-	rs := state.Resource(addr.ContainingResource())
+	rs := ss.Resource(addr.ContainingResource())
-	is := state.ResourceInstance(addr)
+	is := ss.ResourceInstance(addr)
 	if is == nil {
 		if allowMissing {
 			return c.allowMissingExit(addr)
@@ -152,13 +152,13 @@ func (c *TaintCommand) Run(args []string) int {
 	}
 
 	obj.Status = states.ObjectTainted
-	state.SetResourceInstanceCurrent(addr, obj, rs.ProviderConfig)
+	ss.SetResourceInstanceCurrent(addr, obj, rs.ProviderConfig)
 
-	if err := st.WriteState(s); err != nil {
+	if err := stateMgr.WriteState(state); err != nil {
 		c.Ui.Error(fmt.Sprintf("Error writing state file: %s", err))
 		return 1
 	}
-	if err := st.PersistState(); err != nil {
+	if err := stateMgr.PersistState(); err != nil {
 		c.Ui.Error(fmt.Sprintf("Error writing state file: %s", err))
 		return 1
 	}

command/taint_test.go

@@ -64,7 +64,7 @@ func TestTaint_lockedState(t *testing.T) {
 	})
 	statePath := testStateFile(t, state)
 
-	unlock, err := testLockState("./testdata", statePath)
+	unlock, err := testLockState(testDataDir, statePath)
 	if err != nil {
 		t.Fatal(err)
 	}

command/untaint.go

@@ -72,7 +72,7 @@ func (c *UntaintCommand) Run(args []string) int {
 
 	// Get the state
 	workspace := c.Workspace()
-	st, err := b.StateMgr(workspace)
+	stateMgr, err := b.StateMgr(workspace)
 	if err != nil {
 		c.Ui.Error(fmt.Sprintf("Failed to load state: %s", err))
 		return 1
@@ -80,21 +80,21 @@ func (c *UntaintCommand) Run(args []string) int {
 
 	if c.stateLock {
 		stateLocker := clistate.NewLocker(context.Background(), c.stateLockTimeout, c.Ui, c.Colorize())
-		if err := stateLocker.Lock(st, "untaint"); err != nil {
+		if err := stateLocker.Lock(stateMgr, "untaint"); err != nil {
 			c.Ui.Error(fmt.Sprintf("Error locking state: %s", err))
 			return 1
 		}
 		defer stateLocker.Unlock(nil)
 	}
 
-	if err := st.RefreshState(); err != nil {
+	if err := stateMgr.RefreshState(); err != nil {
 		c.Ui.Error(fmt.Sprintf("Failed to load state: %s", err))
 		return 1
 	}
 
 	// Get the actual state structure
-	s := st.State()
+	state := stateMgr.State()
-	if s.Empty() {
+	if state.Empty() {
 		if allowMissing {
 			return c.allowMissingExit(addr)
 		}
@@ -108,11 +108,11 @@ func (c *UntaintCommand) Run(args []string) int {
 		return 1
 	}
 
-	state := s.SyncWrapper()
+	ss := state.SyncWrapper()
 
 	// Get the resource and instance we're going to taint
-	rs := state.Resource(addr.ContainingResource())
+	rs := ss.Resource(addr.ContainingResource())
-	is := state.ResourceInstance(addr)
+	is := ss.ResourceInstance(addr)
 	if is == nil {
 		if allowMissing {
 			return c.allowMissingExit(addr)
@@ -157,13 +157,13 @@ func (c *UntaintCommand) Run(args []string) int {
 		return 1
 	}
 	obj.Status = states.ObjectReady
-	state.SetResourceInstanceCurrent(addr, obj, rs.ProviderConfig)
+	ss.SetResourceInstanceCurrent(addr, obj, rs.ProviderConfig)
 
-	if err := st.WriteState(s); err != nil {
+	if err := stateMgr.WriteState(state); err != nil {
 		c.Ui.Error(fmt.Sprintf("Error writing state file: %s", err))
 		return 1
 	}
-	if err := st.PersistState(); err != nil {
+	if err := stateMgr.PersistState(); err != nil {
 		c.Ui.Error(fmt.Sprintf("Error writing state file: %s", err))
 		return 1
 	}

command/untaint_test.go

@@ -67,7 +67,7 @@ func TestUntaint_lockedState(t *testing.T) {
 		)
 	})
 	statePath := testStateFile(t, state)
-	unlock, err := testLockState("./testdata", statePath)
+	unlock, err := testLockState(testDataDir, statePath)
 	if err != nil {
 		t.Fatal(err)
 	}