From b2ba650c2132abaa9720e82e6fbb60a5ac085cc4 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Wed, 3 Feb 2021 15:05:05 -0500 Subject: [PATCH] cli: Remove deprecated destroy -force flag This dramatically simplifies the logic around auto-approve, which is nice. Also add test coverage for the manual approve step, for both apply and destroy, answering both yes and no. --- backend/backend.go | 11 ++- backend/local/backend_apply.go | 2 +- backend/remote/backend_apply.go | 3 +- command/apply.go | 8 +-- command/apply_destroy_test.go | 118 ++++++++++++++++++++++++++++++++ command/apply_test.go | 85 +++++++++++++++++++++++ 6 files changed, 211 insertions(+), 16 deletions(-) diff --git a/backend/backend.go b/backend/backend.go index 0394ae291..b929aa393 100644 --- a/backend/backend.go +++ b/backend/backend.go @@ -188,12 +188,11 @@ type Operation struct { // The options below are more self-explanatory and affect the runtime // behavior of the operation. - AutoApprove bool - Destroy bool - DestroyForce bool - Parallelism int - Targets []addrs.Targetable - Variables map[string]UnparsedVariableValue + AutoApprove bool + Destroy bool + Parallelism int + Targets []addrs.Targetable + Variables map[string]UnparsedVariableValue // Some operations use root module variables only opportunistically or // don't need them at all. If this flag is set, the backend must treat diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index 7b3eb1285..cd018269a 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -79,7 +79,7 @@ func (b *Local) opApply( trivialPlan := plan.Changes.Empty() hasUI := op.UIOut != nil && op.UIIn != nil - mustConfirm := hasUI && ((op.Destroy && (!op.DestroyForce && !op.AutoApprove)) || (!op.Destroy && !op.AutoApprove && !trivialPlan)) + mustConfirm := hasUI && !op.AutoApprove && !trivialPlan if mustConfirm { var desc, query string if op.Destroy { diff --git a/backend/remote/backend_apply.go b/backend/remote/backend_apply.go index 5db412baa..f03323d24 100644 --- a/backend/remote/backend_apply.go +++ b/backend/remote/backend_apply.go @@ -170,8 +170,7 @@ func (b *Remote) opApply(stopCtx, cancelCtx context.Context, op *backend.Operati return r, diags.Err() } - mustConfirm := (op.UIIn != nil && op.UIOut != nil) && - ((op.Destroy && (!op.DestroyForce && !op.AutoApprove)) || (!op.Destroy && !op.AutoApprove)) + mustConfirm := (op.UIIn != nil && op.UIOut != nil) && !op.AutoApprove if !w.AutoApply { if mustConfirm { diff --git a/command/apply.go b/command/apply.go index 3d7515341..3a4106590 100644 --- a/command/apply.go +++ b/command/apply.go @@ -24,7 +24,7 @@ type ApplyCommand struct { } func (c *ApplyCommand) Run(args []string) int { - var destroyForce, refresh, autoApprove bool + var refresh, autoApprove bool args = c.Meta.process(args) cmdName := "apply" if c.Destroy { @@ -33,9 +33,6 @@ func (c *ApplyCommand) Run(args []string) int { cmdFlags := c.Meta.extendedFlagSet(cmdName) cmdFlags.BoolVar(&autoApprove, "auto-approve", false, "skip interactive approval of plan before applying") - if c.Destroy { - cmdFlags.BoolVar(&destroyForce, "force", false, "deprecated: same as auto-approve") - } cmdFlags.BoolVar(&refresh, "refresh", true, "refresh") cmdFlags.IntVar(&c.Meta.parallelism, "parallelism", DefaultParallelism, "parallelism") cmdFlags.StringVar(&c.Meta.statePath, "state", "", "path") @@ -160,7 +157,6 @@ func (c *ApplyCommand) Run(args []string) int { opReq.AutoApprove = autoApprove opReq.ConfigDir = configPath opReq.Destroy = c.Destroy - opReq.DestroyForce = destroyForce opReq.PlanFile = planFile opReq.PlanRefresh = refresh opReq.Type = backend.OperationTypeApply @@ -319,8 +315,6 @@ Options: -auto-approve Skip interactive approval before destroying. - -force Deprecated: same as auto-approve. - -lock=true Lock the state file when locking is supported. -lock-timeout=0s Duration to retry a state lock. diff --git a/command/apply_destroy_test.go b/command/apply_destroy_test.go index cbe14ae4e..a5a581868 100644 --- a/command/apply_destroy_test.go +++ b/command/apply_destroy_test.go @@ -1,6 +1,7 @@ package command import ( + "bytes" "os" "strings" "testing" @@ -114,6 +115,123 @@ func TestApply_destroy(t *testing.T) { } } +func TestApply_destroyApproveNo(t *testing.T) { + // Create a temporary working directory that is empty + td := tempDir(t) + testCopyDir(t, testFixturePath("apply"), td) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + // Create some existing state + originalState := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_instance", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + AttrsJSON: []byte(`{"id":"bar"}`), + Status: states.ObjectReady, + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + }) + statePath := testStateFile(t, originalState) + + // Disable test mode so input would be asked + test = false + defer func() { test = true }() + + // Answer approval request with "no" + defaultInputReader = bytes.NewBufferString("no\n") + defaultInputWriter = new(bytes.Buffer) + + p := applyFixtureProvider() + ui := new(cli.MockUi) + c := &ApplyCommand{ + Destroy: true, + Meta: Meta{ + testingOverrides: metaOverridesForProvider(p), + Ui: ui, + }, + } + + args := []string{ + "-state", statePath, + } + if code := c.Run(args); code != 1 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + if got, want := ui.OutputWriter.String(), "Destroy cancelled"; !strings.Contains(got, want) { + t.Fatalf("expected output to include %q, but was:\n%s", want, got) + } + + state := testStateRead(t, statePath) + if state == nil { + t.Fatal("state should not be nil") + } + actualStr := strings.TrimSpace(state.String()) + expectedStr := strings.TrimSpace(originalState.String()) + if actualStr != expectedStr { + t.Fatalf("bad:\n\n%s\n\n%s", actualStr, expectedStr) + } +} + +func TestApply_destroyApproveYes(t *testing.T) { + // Create a temporary working directory that is empty + td := tempDir(t) + testCopyDir(t, testFixturePath("apply"), td) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + statePath := testTempFile(t) + + p := applyFixtureProvider() + + // Disable test mode so input would be asked + test = false + defer func() { test = true }() + + // Answer approval request with "yes" + defaultInputReader = bytes.NewBufferString("yes\n") + defaultInputWriter = new(bytes.Buffer) + + ui := new(cli.MockUi) + c := &ApplyCommand{ + Destroy: true, + Meta: Meta{ + testingOverrides: metaOverridesForProvider(p), + Ui: ui, + }, + } + + args := []string{ + "-state", statePath, + } + if code := c.Run(args); code != 0 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + + if _, err := os.Stat(statePath); err != nil { + t.Fatalf("err: %s", err) + } + + state := testStateRead(t, statePath) + if state == nil { + t.Fatal("state should not be nil") + } + + actualStr := strings.TrimSpace(state.String()) + expectedStr := strings.TrimSpace(testApplyDestroyStr) + if actualStr != expectedStr { + t.Fatalf("bad:\n\n%s\n\n%s", actualStr, expectedStr) + } +} + func TestApply_destroyLockedState(t *testing.T) { originalState := states.BuildState(func(s *states.SyncState) { s.SetResourceInstanceCurrent( diff --git a/command/apply_test.go b/command/apply_test.go index dbfad3ab0..5e1f0329b 100644 --- a/command/apply_test.go +++ b/command/apply_test.go @@ -63,6 +63,91 @@ func TestApply(t *testing.T) { } } +func TestApply_approveNo(t *testing.T) { + // Create a temporary working directory that is empty + td := tempDir(t) + testCopyDir(t, testFixturePath("apply"), td) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + statePath := testTempFile(t) + + // Disable test mode so input would be asked + test = false + defer func() { test = true }() + + // Answer approval request with "no" + defaultInputReader = bytes.NewBufferString("no\n") + defaultInputWriter = new(bytes.Buffer) + + p := applyFixtureProvider() + ui := new(cli.MockUi) + c := &ApplyCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(p), + Ui: ui, + }, + } + + args := []string{ + "-state", statePath, + } + if code := c.Run(args); code != 1 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + if got, want := ui.OutputWriter.String(), "Apply cancelled"; !strings.Contains(got, want) { + t.Fatalf("expected output to include %q, but was:\n%s", want, got) + } + + if _, err := os.Stat(statePath); err == nil || !os.IsNotExist(err) { + t.Fatalf("state file should not exist") + } +} + +func TestApply_approveYes(t *testing.T) { + // Create a temporary working directory that is empty + td := tempDir(t) + testCopyDir(t, testFixturePath("apply"), td) + defer os.RemoveAll(td) + defer testChdir(t, td)() + + statePath := testTempFile(t) + + p := applyFixtureProvider() + + // Disable test mode so input would be asked + test = false + defer func() { test = true }() + + // Answer approval request with "yes" + defaultInputReader = bytes.NewBufferString("yes\n") + defaultInputWriter = new(bytes.Buffer) + + ui := new(cli.MockUi) + c := &ApplyCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(p), + Ui: ui, + }, + } + + args := []string{ + "-state", statePath, + } + if code := c.Run(args); code != 0 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + + if _, err := os.Stat(statePath); err != nil { + t.Fatalf("err: %s", err) + } + + state := testStateRead(t, statePath) + if state == nil { + t.Fatal("state should not be nil") + } +} + // test apply with locked state func TestApply_lockedState(t *testing.T) { // Create a temporary working directory that is empty