From 5f939b42fe14ec5e75437398789892fb4972c231 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 23 Jun 2017 14:39:37 -0400 Subject: [PATCH 1/3] test that `state mv -state` doesn't use Backend If we provide a -state flag to a state command, we do not want terraform to modify the backend state. This test fails since the state specified in the backend doesn't exist --- command/state_mv_test.go | 81 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/command/state_mv_test.go b/command/state_mv_test.go index 983d67b8d..fe8102880 100644 --- a/command/state_mv_test.go +++ b/command/state_mv_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "github.com/hashicorp/terraform/helper/copy" "github.com/hashicorp/terraform/terraform" "github.com/mitchellh/cli" ) @@ -72,6 +73,86 @@ func TestStateMv(t *testing.T) { testStateOutput(t, backups[0], testStateMvOutputOriginal) } +// don't modify backend state is we supply a -state flag +func TestStateMv_explicitWithBackend(t *testing.T) { + td := tempDir(t) + copy.CopyDir(testFixturePath("init-backend"), td) + defer os.RemoveAll(td) + defer testChdir(t, 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) + + // init our backend + ui := new(cli.MockUi) + ic := &InitCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(testProvider()), + Ui: ui, + }, + } + + args := []string{} + if code := ic.Run(args); code != 0 { + t.Fatalf("bad: \n%s", ui.ErrorWriter.String()) + } + + // only modify statePath + p := testProvider() + ui = new(cli.MockUi) + c := &StateMvCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(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) +} + func TestStateMv_backupExplicit(t *testing.T) { td := tempDir(t) defer os.RemoveAll(td) From 6e7baaaeffdcce8370fd4e51e765c3cb9a792158 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 23 Jun 2017 14:41:49 -0400 Subject: [PATCH 2/3] don't load the backend when -state is provided When using a `state` command, if the `-state` flag is provided we do not want to modify the Backend state. In this case we should always create a local state instance. The backup flag was also being ignored, and some tests were relying on that, which have been fixed. --- command/state_meta.go | 81 ++++++++++++++++++++++++---------------- command/state_mv_test.go | 7 +--- command/state_rm_test.go | 7 +--- 3 files changed, 51 insertions(+), 44 deletions(-) diff --git a/command/state_meta.go b/command/state_meta.go index 2ac16385b..dc17fa073 100644 --- a/command/state_meta.go +++ b/command/state_meta.go @@ -18,46 +18,63 @@ type StateMeta struct{} // backups to be timestamped rather than just the original state path plus a // backup path. func (c *StateMeta) State(m *Meta) (state.State, error) { - // Load the backend - b, err := m.Backend(nil) - if err != nil { - return nil, err + var realState state.State + backupPath := m.backupPath + stateOutPath := m.statePath + + // use the specified state + if m.statePath != "" { + realState = &state.LocalState{ + Path: m.statePath, + } + } else { + // Load the backend + b, err := m.Backend(nil) + if err != nil { + return nil, err + } + + env := m.Workspace() + // Get the state + s, err := b.State(env) + if err != nil { + return nil, err + } + + // Get a local backend + localRaw, err := m.Backend(&BackendOpts{ForceLocal: true}) + if err != nil { + // This should never fail + panic(err) + } + localB := localRaw.(*backendlocal.Local) + _, stateOutPath, _ = localB.StatePaths(env) + if err != nil { + return nil, err + } + + realState = s } - env := m.Workspace() - // Get the state - s, err := b.State(env) - if err != nil { - return nil, err + // We always backup state commands, so set the back if none was specified + // (the default is "-", but some tests bypass the flag parsing). + if backupPath == "-" || backupPath == "" { + // Determine the backup path. stateOutPath is set to the resulting + // file where state is written (cached in the case of remote state) + backupPath = fmt.Sprintf( + "%s.%d%s", + stateOutPath, + time.Now().UTC().Unix(), + DefaultBackupExtension) } - // Get a local backend - localRaw, err := m.Backend(&BackendOpts{ForceLocal: true}) - if err != nil { - // This should never fail - panic(err) - } - localB := localRaw.(*backendlocal.Local) - _, stateOutPath, _ := localB.StatePaths(env) - if err != nil { - return nil, err - } - - // Determine the backup path. stateOutPath is set to the resulting - // file where state is written (cached in the case of remote state) - backupPath := fmt.Sprintf( - "%s.%d%s", - stateOutPath, - time.Now().UTC().Unix(), - DefaultBackupExtension) - // Wrap it for backups - s = &state.BackupState{ - Real: s, + realState = &state.BackupState{ + Real: realState, Path: backupPath, } - return s, nil + return realState, nil } // filterInstance filters a single instance out of filter results. diff --git a/command/state_mv_test.go b/command/state_mv_test.go index fe8102880..d3f05b31b 100644 --- a/command/state_mv_test.go +++ b/command/state_mv_test.go @@ -213,12 +213,7 @@ func TestStateMv_backupExplicit(t *testing.T) { // 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) + // Test backup testStateOutput(t, backupPath, testStateMvOutputOriginal) } diff --git a/command/state_rm_test.go b/command/state_rm_test.go index 22256366c..559ed09a9 100644 --- a/command/state_rm_test.go +++ b/command/state_rm_test.go @@ -130,12 +130,7 @@ func TestStateRm_backupExplicit(t *testing.T) { // 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) + // Test backup testStateOutput(t, backupPath, testStateRmOutputOriginal) } From 833cc9a6c5777e9f91ee3c635c78624eb2098388 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 23 Jun 2017 14:46:09 -0400 Subject: [PATCH 3/3] Fix state mv/rm -backup documentation There is only one backup made, contrary to the help text. We may want to implement that, but the documentation should match the current behavior. --- command/state_mv.go | 3 +-- command/state_rm.go | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/command/state_mv.go b/command/state_mv.go index 5b99dd2ab..71e934af4 100644 --- a/command/state_mv.go +++ b/command/state_mv.go @@ -203,8 +203,7 @@ 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. This backup will be made in addition - to the timestamped backup. + a backup extension. -backup-out=PATH Path where Terraform should write the backup for the destination state. This can't be disabled. If not set, Terraform diff --git a/command/state_rm.go b/command/state_rm.go index b2491c4e8..44c8d8a05 100644 --- a/command/state_rm.go +++ b/command/state_rm.go @@ -78,8 +78,7 @@ 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. This backup will be made in addition - to the timestamped backup. + a backup extension. -state=statefile Path to a Terraform state file to use to look up Terraform-managed resources. By default it will