command/state: mv and rm -backup works

Fixes #12154

The "-backup" flag before for "state *" CLI had some REALLY bizarre behavior:
it would change the _destination_ state and actually not create any
additional backup at all (the original state was unchanged and the
normal timestamped backup still are written). Really weird.

This PR makes the -backup flag work as you'd expect with one caveat:
we'll _still_ create the timestamped backup file. The timestamped backup
file helps make sure that you always get a backup history when using
these commands. We don't want to make it easy for you to overwrite a
state with the `-backup` flag.

command/state_meta.go

@@ -17,9 +17,6 @@ type StateMeta struct{}
 // in the way that backups are done. This configures backups to be timestamped
 // rather than just the original state path plus a backup path.
 func (c *StateMeta) State(m *Meta) (state.State, error) {
-	// Disable backups since we wrap it manually below
-	m.backupPath = "-"
-
 	// Load the backend
 	b, err := m.Backend(nil)
 	if err != nil {

command/state_mv.go

@@ -20,9 +20,9 @@ func (c *StateMvCommand) Run(args []string) int {
 	// We create two metas to track the two states
 	var meta1, meta2 Meta
 	cmdFlags := c.Meta.flagSet("state mv")
-	cmdFlags.StringVar(&meta1.stateOutPath, "backup", "", "backup")
+	cmdFlags.StringVar(&meta1.backupPath, "backup", "-", "backup")
 	cmdFlags.StringVar(&meta1.statePath, "state", DefaultStateFilename, "path")
-	cmdFlags.StringVar(&meta2.stateOutPath, "backup-out", "", "backup")
+	cmdFlags.StringVar(&meta2.backupPath, "backup-out", "-", "backup")
 	cmdFlags.StringVar(&meta2.statePath, "state-out", "", "path")
 	if err := cmdFlags.Parse(args); err != nil {
 		return cli.RunResultHelp
@@ -193,7 +193,8 @@ Options:
   -backup=PATH        Path where Terraform should write the backup for the original
                       state. This can't be disabled. If not set, Terraform
                       will write it to the same path as the statefile with
-                      a backup extension.
+                      a backup extension. This backup will be made in addition
+                      to the timestamped backup.
 
   -backup-out=PATH    Path where Terraform should write the backup for the destination
                       state. This can't be disabled. If not set, Terraform

command/state_mv_test.go

@@ -1,6 +1,7 @@
 package command
 
 import (
+	"os"
 	"path/filepath"
 	"testing"
 
@@ -71,6 +72,75 @@ func TestStateMv(t *testing.T) {
 	testStateOutput(t, backups[0], testStateMvOutputOriginal)
 }
 
+func TestStateMv_backupExplicit(t *testing.T) {
+	td := tempDir(t)
+	defer os.RemoveAll(td)
+	backupPath := filepath.Join(td, "backup")
+
+	state := &terraform.State{
+		Modules: []*terraform.ModuleState{
+			&terraform.ModuleState{
+				Path: []string{"root"},
+				Resources: map[string]*terraform.ResourceState{
+					"test_instance.foo": &terraform.ResourceState{
+						Type: "test_instance",
+						Primary: &terraform.InstanceState{
+							ID: "bar",
+							Attributes: map[string]string{
+								"foo": "value",
+								"bar": "value",
+							},
+						},
+					},
+
+					"test_instance.baz": &terraform.ResourceState{
+						Type: "test_instance",
+						Primary: &terraform.InstanceState{
+							ID: "foo",
+							Attributes: map[string]string{
+								"foo": "value",
+								"bar": "value",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	statePath := testStateFile(t, state)
+
+	p := testProvider()
+	ui := new(cli.MockUi)
+	c := &StateMvCommand{
+		Meta: Meta{
+			ContextOpts: testCtxConfig(p),
+			Ui:          ui,
+		},
+	}
+
+	args := []string{
+		"-backup", backupPath,
+		"-state", statePath,
+		"test_instance.foo",
+		"test_instance.bar",
+	}
+	if code := c.Run(args); code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	}
+
+	// Test it is correct
+	testStateOutput(t, statePath, testStateMvOutput)
+
+	// Test we have backups
+	backups := testStateBackups(t, filepath.Dir(statePath))
+	if len(backups) != 1 {
+		t.Fatalf("bad: %#v", backups)
+	}
+	testStateOutput(t, backups[0], testStateMvOutputOriginal)
+	testStateOutput(t, backupPath, testStateMvOutputOriginal)
+}
+
 func TestStateMv_stateOutNew(t *testing.T) {
 	state := &terraform.State{
 		Modules: []*terraform.ModuleState{

command/state_rm.go

@@ -16,9 +16,8 @@ type StateRmCommand struct {
 func (c *StateRmCommand) Run(args []string) int {
 	args = c.Meta.process(args, true)
 
-	var backupPath string
 	cmdFlags := c.Meta.flagSet("state show")
-	cmdFlags.StringVar(&backupPath, "backup", "", "backup")
+	cmdFlags.StringVar(&c.Meta.backupPath, "backup", "-", "backup")
 	cmdFlags.StringVar(&c.Meta.statePath, "state", DefaultStateFilename, "path")
 	if err := cmdFlags.Parse(args); err != nil {
 		return cli.RunResultHelp
@@ -75,7 +74,8 @@ Options:
   -backup=PATH        Path where Terraform should write the backup
                       state. This can't be disabled. If not set, Terraform
                       will write it to the same path as the statefile with
-                      a backup extension.
+                      a backup extension. This backup will be made in addition
+                      to the timestamped backup.
 
   -state=statefile    Path to a Terraform state file to use to look
                       up Terraform-managed resources. By default it will

command/state_rm_test.go

@@ -1,6 +1,7 @@
 package command
 
 import (
+	"os"
 	"path/filepath"
 	"testing"
 
@@ -70,6 +71,74 @@ func TestStateRm(t *testing.T) {
 	testStateOutput(t, backups[0], testStateRmOutputOriginal)
 }
 
+func TestStateRm_backupExplicit(t *testing.T) {
+	td := tempDir(t)
+	defer os.RemoveAll(td)
+	backupPath := filepath.Join(td, "backup")
+
+	state := &terraform.State{
+		Modules: []*terraform.ModuleState{
+			&terraform.ModuleState{
+				Path: []string{"root"},
+				Resources: map[string]*terraform.ResourceState{
+					"test_instance.foo": &terraform.ResourceState{
+						Type: "test_instance",
+						Primary: &terraform.InstanceState{
+							ID: "bar",
+							Attributes: map[string]string{
+								"foo": "value",
+								"bar": "value",
+							},
+						},
+					},
+
+					"test_instance.bar": &terraform.ResourceState{
+						Type: "test_instance",
+						Primary: &terraform.InstanceState{
+							ID: "foo",
+							Attributes: map[string]string{
+								"foo": "value",
+								"bar": "value",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	statePath := testStateFile(t, state)
+
+	p := testProvider()
+	ui := new(cli.MockUi)
+	c := &StateRmCommand{
+		Meta: Meta{
+			ContextOpts: testCtxConfig(p),
+			Ui:          ui,
+		},
+	}
+
+	args := []string{
+		"-backup", backupPath,
+		"-state", statePath,
+		"test_instance.foo",
+	}
+	if code := c.Run(args); code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	}
+
+	// Test it is correct
+	testStateOutput(t, statePath, testStateRmOutput)
+
+	// Test we have backups
+	backups := testStateBackups(t, filepath.Dir(statePath))
+	if len(backups) != 1 {
+		t.Fatalf("bad: %#v", backups)
+	}
+	testStateOutput(t, backups[0], testStateRmOutputOriginal)
+	testStateOutput(t, backupPath, testStateRmOutputOriginal)
+}
+
 func TestStateRm_noState(t *testing.T) {
 	tmp, cwd := testCwd(t)
 	defer testFixCwd(t, tmp, cwd)