From 5ca118b4e6ba8e85fc667403117c483f6beb0d5f Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Fri, 29 Jan 2021 14:39:06 -0500 Subject: [PATCH] cli: Move resource count code to command package CountHook is an implementation of terraform.Hook which is used to calculate how many resources were added, changed, or destroyed during an apply. This hook was previously injected in the local backend code, which means that the apply command code has no access to these counts. This commit moves the CountHook code into the command package, and removes an unused instance of the hook in the plan code path. The goal here is moving UI code into the command package. --- backend/local/backend_apply.go | 33 +------------- backend/local/backend_plan.go | 5 --- backend/local/backend_plan_test.go | 44 ------------------- backend/local/counthookaction_string.go | 25 ----------- backend/local/hook_count_action.go | 11 ----- backend/local/testdata/plan-scaleout/main.tf | 10 ----- backend/remote/backend_apply_test.go | 31 ++++++++----- command/apply.go | 32 ++++++++++++++ {backend/local => command}/hook_count.go | 2 +- {backend/local => command}/hook_count_test.go | 2 +- 10 files changed, 55 insertions(+), 140 deletions(-) delete mode 100644 backend/local/counthookaction_string.go delete mode 100644 backend/local/hook_count_action.go delete mode 100644 backend/local/testdata/plan-scaleout/main.tf rename {backend/local => command}/hook_count.go (99%) rename {backend/local => command}/hook_count_test.go (99%) diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index 10c1bf248..7b3eb1285 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -39,15 +39,13 @@ func (b *Local) opApply( return } - // Set up our count hook that keeps track of resource changes - countHook := new(CountHook) stateHook := new(StateHook) if b.ContextOpts == nil { b.ContextOpts = new(terraform.ContextOpts) } old := b.ContextOpts.Hooks defer func() { b.ContextOpts.Hooks = old }() - b.ContextOpts.Hooks = append(b.ContextOpts.Hooks, countHook, stateHook) + b.ContextOpts.Hooks = append(b.ContextOpts.Hooks, stateHook) // Get our context tfCtx, _, opState, contextDiags := b.context(op) @@ -183,35 +181,6 @@ func (b *Local) opApply( // here just before we show the summary and next steps. If we encountered // errors then we would've returned early at some other point above. b.ShowDiagnostics(diags) - - // If we have a UI, output the results - if b.CLI != nil { - if op.Destroy { - b.CLI.Output(b.Colorize().Color(fmt.Sprintf( - "[reset][bold][green]\n"+ - "Destroy complete! Resources: %d destroyed.", - countHook.Removed))) - } else { - b.CLI.Output(b.Colorize().Color(fmt.Sprintf( - "[reset][bold][green]\n"+ - "Apply complete! Resources: %d added, %d changed, %d destroyed.", - countHook.Added, - countHook.Changed, - countHook.Removed))) - } - - // only show the state file help message if the state is local. - if (countHook.Added > 0 || countHook.Changed > 0) && b.StateOutPath != "" { - b.CLI.Output(b.Colorize().Color(fmt.Sprintf( - "[reset]\n"+ - "The state of your infrastructure has been saved to the path\n"+ - "below. This state is required to modify and destroy your\n"+ - "infrastructure, so keep it safe. To inspect the complete state\n"+ - "use the `terraform show` command.\n\n"+ - "State path: %s", - b.StateOutPath))) - } - } } // backupStateForError is called in a scenario where we're unable to persist the diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go index 35bac8bd5..3a45ec607 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -59,14 +59,9 @@ func (b *Local) opPlan( return } - // Set up our count hook that keeps track of resource changes - countHook := new(CountHook) if b.ContextOpts == nil { b.ContextOpts = new(terraform.ContextOpts) } - old := b.ContextOpts.Hooks - defer func() { b.ContextOpts.Hooks = old }() - b.ContextOpts.Hooks = append(b.ContextOpts.Hooks, countHook) // Get our context tfCtx, configSnap, opState, ctxDiags := b.context(op) diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go index 0b0b14172..4e0bc85a3 100644 --- a/backend/local/backend_plan_test.go +++ b/backend/local/backend_plan_test.go @@ -4,7 +4,6 @@ import ( "context" "os" "path/filepath" - "reflect" "strings" "testing" @@ -728,49 +727,6 @@ func TestLocal_planOutPathNoChange(t *testing.T) { } } -// TestLocal_planScaleOutNoDupeCount tests a Refresh/Plan sequence when a -// resource count is scaled out. The scaled out node needs to exist in the -// graph and run through a plan-style sequence during the refresh phase, but -// can conflate the count if its post-diff count hooks are not skipped. This -// checks to make sure the correct resource count is ultimately given to the -// UI. -func TestLocal_planScaleOutNoDupeCount(t *testing.T) { - b, cleanup := TestLocal(t) - defer cleanup() - TestLocalProvider(t, b, "test", planFixtureSchema()) - testStateFile(t, b.StatePath, testPlanState()) - - actual := new(CountHook) - b.ContextOpts.Hooks = append(b.ContextOpts.Hooks, actual) - - outDir := testTempDir(t) - defer os.RemoveAll(outDir) - - op, configCleanup := testOperationPlan(t, "./testdata/plan-scaleout") - defer configCleanup() - op.PlanRefresh = true - - run, err := b.Operation(context.Background(), op) - if err != nil { - t.Fatalf("bad: %s", err) - } - <-run.Done() - if run.Result != backend.OperationSuccess { - t.Fatalf("plan operation failed") - } - - expected := new(CountHook) - expected.ToAdd = 1 - expected.ToChange = 0 - expected.ToRemoveAndAdd = 0 - expected.ToRemove = 0 - - if !reflect.DeepEqual(expected, actual) { - t.Fatalf("Expected %#v, got %#v instead.", - expected, actual) - } -} - func testOperationPlan(t *testing.T, configDir string) (*backend.Operation, func()) { t.Helper() diff --git a/backend/local/counthookaction_string.go b/backend/local/counthookaction_string.go deleted file mode 100644 index 591004749..000000000 --- a/backend/local/counthookaction_string.go +++ /dev/null @@ -1,25 +0,0 @@ -// Code generated by "stringer -type=countHookAction hook_count_action.go"; DO NOT EDIT. - -package local - -import "strconv" - -func _() { - // An "invalid array index" compiler error signifies that the constant values have changed. - // Re-run the stringer command to generate them again. - var x [1]struct{} - _ = x[countHookActionAdd-0] - _ = x[countHookActionChange-1] - _ = x[countHookActionRemove-2] -} - -const _countHookAction_name = "countHookActionAddcountHookActionChangecountHookActionRemove" - -var _countHookAction_index = [...]uint8{0, 18, 39, 60} - -func (i countHookAction) String() string { - if i >= countHookAction(len(_countHookAction_index)-1) { - return "countHookAction(" + strconv.FormatInt(int64(i), 10) + ")" - } - return _countHookAction_name[_countHookAction_index[i]:_countHookAction_index[i+1]] -} diff --git a/backend/local/hook_count_action.go b/backend/local/hook_count_action.go deleted file mode 100644 index 9adcd9047..000000000 --- a/backend/local/hook_count_action.go +++ /dev/null @@ -1,11 +0,0 @@ -package local - -//go:generate go run golang.org/x/tools/cmd/stringer -type=countHookAction hook_count_action.go - -type countHookAction byte - -const ( - countHookActionAdd countHookAction = iota - countHookActionChange - countHookActionRemove -) diff --git a/backend/local/testdata/plan-scaleout/main.tf b/backend/local/testdata/plan-scaleout/main.tf deleted file mode 100644 index 4fc97bafa..000000000 --- a/backend/local/testdata/plan-scaleout/main.tf +++ /dev/null @@ -1,10 +0,0 @@ -resource "test_instance" "foo" { - count = 2 - ami = "bar" - - # This is here because at some point it caused a test failure - network_interface { - device_index = 0 - description = "Main network interface" - } -} diff --git a/backend/remote/backend_apply_test.go b/backend/remote/backend_apply_test.go index cc933b4af..c39587867 100644 --- a/backend/remote/backend_apply_test.go +++ b/backend/remote/backend_apply_test.go @@ -775,8 +775,8 @@ func TestRemote_applyForceLocal(t *testing.T) { if !strings.Contains(output, "1 to add, 0 to change, 0 to destroy") { t.Fatalf("expected plan summery in output: %s", output) } - if !strings.Contains(output, "1 added, 0 changed, 0 destroyed") { - t.Fatalf("expected apply summery in output: %s", output) + if !run.State.HasResources() { + t.Fatalf("expected resources in state") } } @@ -833,8 +833,8 @@ func TestRemote_applyWorkspaceWithoutOperations(t *testing.T) { if !strings.Contains(output, "1 to add, 0 to change, 0 to destroy") { t.Fatalf("expected plan summery in output: %s", output) } - if !strings.Contains(output, "1 added, 0 changed, 0 destroyed") { - t.Fatalf("expected apply summery in output: %s", output) + if !run.State.HasResources() { + t.Fatalf("expected resources in state") } } @@ -1396,13 +1396,22 @@ func TestRemote_applyVersionCheck(t *testing.T) { } output := b.CLI.(*cli.MockUi).OutputWriter.String() hasRemote := strings.Contains(output, "Running apply in the remote backend") - if !tc.forceLocal && tc.hasOperations && !hasRemote { - t.Fatalf("missing remote backend header in output: %s", output) - } else if (tc.forceLocal || !tc.hasOperations) && hasRemote { - t.Fatalf("unexpected remote backend header in output: %s", output) - } - if !strings.Contains(output, "1 added, 0 changed, 0 destroyed") { - t.Fatalf("expected apply summary in output: %s", output) + hasSummary := strings.Contains(output, "1 added, 0 changed, 0 destroyed") + hasResources := run.State.HasResources() + if !tc.forceLocal && tc.hasOperations { + if !hasRemote { + t.Errorf("missing remote backend header in output: %s", output) + } + if !hasSummary { + t.Errorf("expected apply summary in output: %s", output) + } + } else { + if hasRemote { + t.Errorf("unexpected remote backend header in output: %s", output) + } + if !hasResources { + t.Errorf("expected resources in state") + } } } }) diff --git a/command/apply.go b/command/apply.go index fae0a58ed..17b83f5fe 100644 --- a/command/apply.go +++ b/command/apply.go @@ -87,6 +87,10 @@ func (c *ApplyCommand) Run(args []string) int { } } + // Set up our count hook that keeps track of resource changes + countHook := new(CountHook) + c.ExtraHooks = append(c.ExtraHooks, countHook) + // Load the backend var be backend.Enhanced var beDiags tfdiags.Diagnostics @@ -172,10 +176,38 @@ func (c *ApplyCommand) Run(args []string) int { c.showDiagnostics(err) return 1 } + if op.Result != backend.OperationSuccess { return op.Result.ExitStatus() } + // Show the count results from the operation + if c.Destroy { + c.Ui.Output(c.Colorize().Color(fmt.Sprintf( + "[reset][bold][green]\n"+ + "Destroy complete! Resources: %d destroyed.", + countHook.Removed))) + } else { + c.Ui.Output(c.Colorize().Color(fmt.Sprintf( + "[reset][bold][green]\n"+ + "Apply complete! Resources: %d added, %d changed, %d destroyed.", + countHook.Added, + countHook.Changed, + countHook.Removed))) + } + + // only show the state file help message if the state is local. + if (countHook.Added > 0 || countHook.Changed > 0) && c.Meta.stateOutPath != "" { + c.Ui.Output(c.Colorize().Color(fmt.Sprintf( + "[reset]\n"+ + "The state of your infrastructure has been saved to the path\n"+ + "below. This state is required to modify and destroy your\n"+ + "infrastructure, so keep it safe. To inspect the complete state\n"+ + "use the `terraform show` command.\n\n"+ + "State path: %s", + c.Meta.stateOutPath))) + } + if !c.Destroy { if outputs := outputsAsString(op.State, true); outputs != "" { c.Ui.Output(c.Colorize().Color(outputs)) diff --git a/backend/local/hook_count.go b/command/hook_count.go similarity index 99% rename from backend/local/hook_count.go rename to command/hook_count.go index f9d619ef2..40a834cd0 100644 --- a/backend/local/hook_count.go +++ b/command/hook_count.go @@ -1,4 +1,4 @@ -package local +package command import ( "sync" diff --git a/backend/local/hook_count_test.go b/command/hook_count_test.go similarity index 99% rename from backend/local/hook_count_test.go rename to command/hook_count_test.go index 2bd0df92d..ec7eba984 100644 --- a/backend/local/hook_count_test.go +++ b/command/hook_count_test.go @@ -1,4 +1,4 @@ -package local +package command import ( "reflect"