Merge pull request #27633 from hashicorp/alisdair/count-hook-refactor

cli: Move resource count code to command package

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

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)

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()
 

backend/local/counthookaction_string.go

@@ -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]]
-}

backend/local/hook_count_action.go

@@ -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
-)

backend/local/testdata/plan-scaleout/main.tf

@@ -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"
-  }
-}

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")
+					}
 				}
 			}
 		})

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))

command/hook_count.go

@@ -1,4 +1,4 @@
-package local
+package command
 
 import (
 	"sync"

command/hook_count_test.go

@@ -1,4 +1,4 @@
-package local
+package command
 
 import (
 	"reflect"