Merge pull request #27711 from hashicorp/alisdair/fix-target-flag-parsing-and-add-tests

cli: Improve error for invalid -target flags

command/apply.go

@@ -42,10 +42,15 @@ func (c *ApplyCommand) Run(args []string) int {
 	cmdFlags.DurationVar(&c.Meta.stateLockTimeout, "lock-timeout", 0, "lock timeout")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
+		c.Ui.Error(fmt.Sprintf("Error parsing command-line flags: %s\n", err.Error()))
 		return 1
 	}
 
-	var diags tfdiags.Diagnostics
+	diags := c.parseTargetFlags()
+	if diags.HasErrors() {
+		c.showDiagnostics(diags)
+		return 1
+	}
 
 	args = cmdFlags.Args()
 	var planPath string

command/apply_destroy_test.go

@@ -358,7 +358,10 @@ func TestApply_destroyPath(t *testing.T) {
 	}
 }
 
-func TestApply_destroyTargeted(t *testing.T) {
+// Config with multiple resources with dependencies, targeting destroy of a
+// root node, expecting all other resources to be destroyed due to
+// dependencies.
+func TestApply_destroyTargetedDependencies(t *testing.T) {
 	// Create a temporary working directory that is empty
 	td := tempDir(t)
 	testCopyDir(t, testFixturePath("apply-destroy-targeted"), td)
@@ -488,6 +491,156 @@ func TestApply_destroyTargeted(t *testing.T) {
 	}
 }
 
+// Config with multiple resources with dependencies, targeting destroy of a
+// leaf node, expecting the other resources to remain.
+func TestApply_destroyTargeted(t *testing.T) {
+	// Create a temporary working directory that is empty
+	td := tempDir(t)
+	testCopyDir(t, testFixturePath("apply-destroy-targeted"), td)
+	defer os.RemoveAll(td)
+	defer testChdir(t, td)()
+
+	originalState := states.BuildState(func(s *states.SyncState) {
+		s.SetResourceInstanceCurrent(
+			addrs.Resource{
+				Mode: addrs.ManagedResourceMode,
+				Type: "test_instance",
+				Name: "foo",
+			}.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance),
+			&states.ResourceInstanceObjectSrc{
+				AttrsJSON: []byte(`{"id":"i-ab123"}`),
+				Status:    states.ObjectReady,
+			},
+			addrs.AbsProviderConfig{
+				Provider: addrs.NewDefaultProvider("test"),
+				Module:   addrs.RootModule,
+			},
+		)
+		s.SetResourceInstanceCurrent(
+			addrs.Resource{
+				Mode: addrs.ManagedResourceMode,
+				Type: "test_load_balancer",
+				Name: "foo",
+			}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
+			&states.ResourceInstanceObjectSrc{
+				AttrsJSON:    []byte(`{"id":"i-abc123"}`),
+				Dependencies: []addrs.ConfigResource{mustResourceAddr("test_instance.foo")},
+				Status:       states.ObjectReady,
+			},
+			addrs.AbsProviderConfig{
+				Provider: addrs.NewDefaultProvider("test"),
+				Module:   addrs.RootModule,
+			},
+		)
+	})
+	wantState := states.BuildState(func(s *states.SyncState) {
+		s.SetResourceInstanceCurrent(
+			addrs.Resource{
+				Mode: addrs.ManagedResourceMode,
+				Type: "test_instance",
+				Name: "foo",
+			}.Instance(addrs.IntKey(0)).Absolute(addrs.RootModuleInstance),
+			&states.ResourceInstanceObjectSrc{
+				AttrsJSON: []byte(`{"id":"i-ab123"}`),
+				Status:    states.ObjectReady,
+			},
+			addrs.AbsProviderConfig{
+				Provider: addrs.NewDefaultProvider("test"),
+				Module:   addrs.RootModule,
+			},
+		)
+	})
+	statePath := testStateFile(t, originalState)
+
+	p := testProvider()
+	p.GetSchemaResponse = &providers.GetSchemaResponse{
+		ResourceTypes: map[string]providers.Schema{
+			"test_instance": {
+				Block: &configschema.Block{
+					Attributes: map[string]*configschema.Attribute{
+						"id": {Type: cty.String, Computed: true},
+					},
+				},
+			},
+			"test_load_balancer": {
+				Block: &configschema.Block{
+					Attributes: map[string]*configschema.Attribute{
+						"id":        {Type: cty.String, Computed: true},
+						"instances": {Type: cty.List(cty.String), Optional: true},
+					},
+				},
+			},
+		},
+	}
+	p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
+		return providers.PlanResourceChangeResponse{
+			PlannedState: req.ProposedNewState,
+		}
+	}
+
+	ui := new(cli.MockUi)
+	c := &ApplyCommand{
+		Destroy: true,
+		Meta: Meta{
+			testingOverrides: metaOverridesForProvider(p),
+			Ui:               ui,
+		},
+	}
+
+	// Run the apply command pointing to our existing state
+	args := []string{
+		"-auto-approve",
+		"-target", "test_load_balancer.foo",
+		"-state", statePath,
+	}
+	if code := c.Run(args); code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	}
+
+	// Verify a new state exists
+	if _, err := os.Stat(statePath); err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	f, err := os.Open(statePath)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	defer f.Close()
+
+	stateFile, err := statefile.Read(f)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	if stateFile == nil || stateFile.State == nil {
+		t.Fatal("state should not be nil")
+	}
+
+	actualStr := strings.TrimSpace(stateFile.State.String())
+	expectedStr := strings.TrimSpace(wantState.String())
+	if actualStr != expectedStr {
+		t.Fatalf("bad:\n\nactual:\n%s\n\nexpected:\nb%s", actualStr, expectedStr)
+	}
+
+	// Should have a backup file
+	f, err = os.Open(statePath + DefaultBackupExtension)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	backupStateFile, err := statefile.Read(f)
+	f.Close()
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	backupActualStr := strings.TrimSpace(backupStateFile.State.String())
+	backupExpectedStr := strings.TrimSpace(originalState.String())
+	if backupActualStr != backupExpectedStr {
+		t.Fatalf("bad:\n\nactual:\n%s\n\nexpected:\nb%s", backupActualStr, backupExpectedStr)
+	}
+}
+
 const testApplyDestroyStr = `
 <no state>
 `

command/apply_test.go

@@ -1702,6 +1702,92 @@ output = test
 	testStateOutput(t, statePath, expected)
 }
 
+// Config with multiple resources, targeting apply of a subset
+func TestApply_targeted(t *testing.T) {
+	td := tempDir(t)
+	testCopyDir(t, testFixturePath("apply-targeted"), td)
+	defer os.RemoveAll(td)
+	defer testChdir(t, td)()
+
+	p := testProvider()
+	p.GetSchemaResponse = &providers.GetSchemaResponse{
+		ResourceTypes: map[string]providers.Schema{
+			"test_instance": {
+				Block: &configschema.Block{
+					Attributes: map[string]*configschema.Attribute{
+						"id": {Type: cty.String, Computed: true},
+					},
+				},
+			},
+		},
+	}
+	p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
+		return providers.PlanResourceChangeResponse{
+			PlannedState: req.ProposedNewState,
+		}
+	}
+
+	ui := new(cli.MockUi)
+	c := &ApplyCommand{
+		Meta: Meta{
+			testingOverrides: metaOverridesForProvider(p),
+			Ui:               ui,
+		},
+	}
+
+	args := []string{
+		"-auto-approve",
+		"-target", "test_instance.foo",
+		"-target", "test_instance.baz",
+	}
+	if code := c.Run(args); code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	}
+
+	if got, want := ui.OutputWriter.String(), "3 added, 0 changed, 0 destroyed"; !strings.Contains(got, want) {
+		t.Fatalf("bad change summary, want %q, got:\n%s", want, got)
+	}
+}
+
+// Diagnostics for invalid -target flags
+func TestApply_targetFlagsDiags(t *testing.T) {
+	testCases := map[string]string{
+		"test_instance.": "Dot must be followed by attribute name.",
+		"test_instance":  "Resource specification must include a resource type and name.",
+	}
+
+	for target, wantDiag := range testCases {
+		t.Run(target, func(t *testing.T) {
+			td := testTempDir(t)
+			defer os.RemoveAll(td)
+			defer testChdir(t, td)()
+
+			ui := new(cli.MockUi)
+			c := &ApplyCommand{
+				Meta: Meta{
+					Ui: ui,
+				},
+			}
+
+			args := []string{
+				"-auto-approve",
+				"-target", target,
+			}
+			if code := c.Run(args); code != 1 {
+				t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+			}
+
+			got := ui.ErrorWriter.String()
+			if !strings.Contains(got, target) {
+				t.Fatalf("bad error output, want %q, got:\n%s", target, got)
+			}
+			if !strings.Contains(got, wantDiag) {
+				t.Fatalf("bad error output, want %q, got:\n%s", wantDiag, got)
+			}
+		})
+	}
+}
+
 // applyFixtureSchema returns a schema suitable for processing the
 // configuration in testdata/apply . This schema should be
 // assigned to a mock provider named "test".

command/flag_kv.go

@@ -3,11 +3,6 @@ package command
 import (
 	"fmt"
 	"strings"
-
-	"github.com/hashicorp/hcl/v2"
-	"github.com/hashicorp/hcl/v2/hclsyntax"
-	"github.com/hashicorp/terraform/addrs"
-	"github.com/hashicorp/terraform/tfdiags"
 )
 
 // FlagStringKV is a flag.Value implementation for parsing user variables
@@ -46,34 +41,3 @@ func (v *FlagStringSlice) Set(raw string) error {
 
 	return nil
 }
-
-// FlagTargetSlice is a flag.Value implementation for parsing target addresses
-// from the command line, such as -target=aws_instance.foo -target=aws_vpc.bar .
-type FlagTargetSlice []addrs.Targetable
-
-func (v *FlagTargetSlice) String() string {
-	return ""
-}
-
-func (v *FlagTargetSlice) Set(raw string) error {
-	// FIXME: This is not an ideal way to deal with this because it requires
-	// us to do parsing in a context where we can't nicely return errors
-	// to the user.
-
-	var diags tfdiags.Diagnostics
-	synthFilename := fmt.Sprintf("-target=%q", raw)
-	traversal, syntaxDiags := hclsyntax.ParseTraversalAbs([]byte(raw), synthFilename, hcl.Pos{Line: 1, Column: 1})
-	diags = diags.Append(syntaxDiags)
-	if syntaxDiags.HasErrors() {
-		return diags.Err()
-	}
-
-	target, targetDiags := addrs.ParseTarget(traversal)
-	diags = diags.Append(targetDiags)
-	if targetDiags.HasErrors() {
-		return diags.Err()
-	}
-
-	*v = append(*v, target.Subject)
-	return nil
-}

command/meta.go

@@ -15,6 +15,8 @@ import (
 	"time"
 
 	plugin "github.com/hashicorp/go-plugin"
+	"github.com/hashicorp/hcl/v2"
+	"github.com/hashicorp/hcl/v2/hclsyntax"
 	"github.com/hashicorp/terraform-svchost/disco"
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/backend"
@@ -164,7 +166,8 @@ type Meta struct {
 	input        bool
 
 	// Targets for this context (private)
-	targets []addrs.Targetable
+	targets     []addrs.Targetable
+	targetFlags []string
 
 	// Internal fields
 	color bool
@@ -523,7 +526,7 @@ func (m *Meta) extendedFlagSet(n string) *flag.FlagSet {
 	f := m.defaultFlagSet(n)
 
 	f.BoolVar(&m.input, "input", true, "input")
-	f.Var((*FlagTargetSlice)(&m.targets), "target", "resource to target")
+	f.Var((*FlagStringSlice)(&m.targetFlags), "target", "resource to target")
 	f.BoolVar(&m.compactWarnings, "compact-warnings", false, "use compact warnings")
 
 	if m.variableArgs.items == nil {
@@ -541,6 +544,43 @@ func (m *Meta) extendedFlagSet(n string) *flag.FlagSet {
 	return f
 }
 
+// parseTargetFlags must be called for any commands supporting -target
+// arguments. This method attempts to parse each -target flag into an
+// addrs.Target, storing in the Meta.targets slice.
+//
+// If any flags cannot be parsed, we rewrap the first error diagnostic with a
+// custom title to clarify the source of the error. The normal approach of
+// directly returning the diags from HCL or the addrs package results in
+// confusing incorrect "source" results when presented.
+func (m *Meta) parseTargetFlags() tfdiags.Diagnostics {
+	var diags tfdiags.Diagnostics
+	m.targets = nil
+	for _, tf := range m.targetFlags {
+		traversal, syntaxDiags := hclsyntax.ParseTraversalAbs([]byte(tf), "", hcl.Pos{Line: 1, Column: 1})
+		if syntaxDiags.HasErrors() {
+			diags = diags.Append(tfdiags.Sourceless(
+				tfdiags.Error,
+				fmt.Sprintf("Invalid target %q", tf),
+				syntaxDiags[0].Detail,
+			))
+			continue
+		}
+
+		target, targetDiags := addrs.ParseTarget(traversal)
+		if targetDiags.HasErrors() {
+			diags = diags.Append(tfdiags.Sourceless(
+				tfdiags.Error,
+				fmt.Sprintf("Invalid target %q", tf),
+				targetDiags[0].Description().Detail,
+			))
+			continue
+		}
+
+		m.targets = append(m.targets, target.Subject)
+	}
+	return diags
+}
+
 // process will process the meta-parameters out of the arguments. This
 // will potentially modify the args in-place. It will return the resulting
 // slice.

command/plan.go

@@ -32,6 +32,13 @@ func (c *PlanCommand) Run(args []string) int {
 	cmdFlags.DurationVar(&c.Meta.stateLockTimeout, "lock-timeout", 0, "lock timeout")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
+		c.Ui.Error(fmt.Sprintf("Error parsing command-line flags: %s\n", err.Error()))
+		return 1
+	}
+
+	diags := c.parseTargetFlags()
+	if diags.HasErrors() {
+		c.showDiagnostics(diags)
 		return 1
 	}
 
@@ -47,8 +54,6 @@ func (c *PlanCommand) Run(args []string) int {
 		return 1
 	}
 
-	var diags tfdiags.Diagnostics
-
 	var backendConfig *configs.Backend
 	var configDiags tfdiags.Diagnostics
 	backendConfig, configDiags = c.loadBackendConfig(configPath)

command/plan_test.go

@@ -901,6 +901,90 @@ func TestPlan_init_required(t *testing.T) {
 	}
 }
 
+// Config with multiple resources, targeting plan of a subset
+func TestPlan_targeted(t *testing.T) {
+	td := tempDir(t)
+	testCopyDir(t, testFixturePath("apply-targeted"), td)
+	defer os.RemoveAll(td)
+	defer testChdir(t, td)()
+
+	p := testProvider()
+	p.GetSchemaResponse = &providers.GetSchemaResponse{
+		ResourceTypes: map[string]providers.Schema{
+			"test_instance": {
+				Block: &configschema.Block{
+					Attributes: map[string]*configschema.Attribute{
+						"id": {Type: cty.String, Computed: true},
+					},
+				},
+			},
+		},
+	}
+	p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
+		return providers.PlanResourceChangeResponse{
+			PlannedState: req.ProposedNewState,
+		}
+	}
+
+	ui := new(cli.MockUi)
+	c := &PlanCommand{
+		Meta: Meta{
+			testingOverrides: metaOverridesForProvider(p),
+			Ui:               ui,
+		},
+	}
+
+	args := []string{
+		"-target", "test_instance.foo",
+		"-target", "test_instance.baz",
+	}
+	if code := c.Run(args); code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	}
+
+	if got, want := ui.OutputWriter.String(), "3 to add, 0 to change, 0 to destroy"; !strings.Contains(got, want) {
+		t.Fatalf("bad change summary, want %q, got:\n%s", want, got)
+	}
+}
+
+// Diagnostics for invalid -target flags
+func TestPlan_targetFlagsDiags(t *testing.T) {
+	testCases := map[string]string{
+		"test_instance.": "Dot must be followed by attribute name.",
+		"test_instance":  "Resource specification must include a resource type and name.",
+	}
+
+	for target, wantDiag := range testCases {
+		t.Run(target, func(t *testing.T) {
+			td := testTempDir(t)
+			defer os.RemoveAll(td)
+			defer testChdir(t, td)()
+
+			ui := new(cli.MockUi)
+			c := &PlanCommand{
+				Meta: Meta{
+					Ui: ui,
+				},
+			}
+
+			args := []string{
+				"-target", target,
+			}
+			if code := c.Run(args); code != 1 {
+				t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+			}
+
+			got := ui.ErrorWriter.String()
+			if !strings.Contains(got, target) {
+				t.Fatalf("bad error output, want %q, got:\n%s", target, got)
+			}
+			if !strings.Contains(got, wantDiag) {
+				t.Fatalf("bad error output, want %q, got:\n%s", wantDiag, got)
+			}
+		})
+	}
+}
+
 // planFixtureSchema returns a schema suitable for processing the
 // configuration in testdata/plan . This schema should be
 // assigned to a mock provider named "test".

command/refresh.go

@@ -25,6 +25,13 @@ func (c *RefreshCommand) Run(args []string) int {
 	cmdFlags.DurationVar(&c.Meta.stateLockTimeout, "lock-timeout", 0, "lock timeout")
 	cmdFlags.Usage = func() { c.Ui.Error(c.Help()) }
 	if err := cmdFlags.Parse(args); err != nil {
+		c.Ui.Error(fmt.Sprintf("Error parsing command-line flags: %s\n", err.Error()))
+		return 1
+	}
+
+	diags := c.parseTargetFlags()
+	if diags.HasErrors() {
+		c.showDiagnostics(diags)
 		return 1
 	}
 
@@ -34,8 +41,6 @@ func (c *RefreshCommand) Run(args []string) int {
 		return 1
 	}
 
-	var diags tfdiags.Diagnostics
-
 	// Check for user-supplied plugin path
 	if c.pluginPath, err = c.loadPluginPath(); err != nil {
 		c.Ui.Error(fmt.Sprintf("Error loading plugin path: %s", err))

command/refresh_test.go

@@ -736,6 +736,97 @@ func TestRefresh_displaysOutputs(t *testing.T) {
 	}
 }
 
+// Config with multiple resources, targeting refresh of a subset
+func TestRefresh_targeted(t *testing.T) {
+	td := tempDir(t)
+	testCopyDir(t, testFixturePath("refresh-targeted"), td)
+	defer os.RemoveAll(td)
+	defer testChdir(t, td)()
+
+	state := testState()
+	statePath := testStateFile(t, state)
+
+	p := testProvider()
+	p.GetSchemaResponse = &providers.GetSchemaResponse{
+		ResourceTypes: map[string]providers.Schema{
+			"test_instance": {
+				Block: &configschema.Block{
+					Attributes: map[string]*configschema.Attribute{
+						"id": {Type: cty.String, Computed: true},
+					},
+				},
+			},
+		},
+	}
+	p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
+		return providers.PlanResourceChangeResponse{
+			PlannedState: req.ProposedNewState,
+		}
+	}
+
+	ui := new(cli.MockUi)
+	c := &RefreshCommand{
+		Meta: Meta{
+			testingOverrides: metaOverridesForProvider(p),
+			Ui:               ui,
+		},
+	}
+
+	args := []string{
+		"-target", "test_instance.foo",
+		"-state", statePath,
+	}
+	if code := c.Run(args); code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+	}
+
+	got := ui.OutputWriter.String()
+	if want := "test_instance.foo: Refreshing"; !strings.Contains(got, want) {
+		t.Fatalf("expected output to contain %q, got:\n%s", want, got)
+	}
+	if doNotWant := "test_instance.bar: Refreshing"; strings.Contains(got, doNotWant) {
+		t.Fatalf("expected output not to contain %q, got:\n%s", doNotWant, got)
+	}
+}
+
+// Diagnostics for invalid -target flags
+func TestRefresh_targetFlagsDiags(t *testing.T) {
+	testCases := map[string]string{
+		"test_instance.": "Dot must be followed by attribute name.",
+		"test_instance":  "Resource specification must include a resource type and name.",
+	}
+
+	for target, wantDiag := range testCases {
+		t.Run(target, func(t *testing.T) {
+			td := testTempDir(t)
+			defer os.RemoveAll(td)
+			defer testChdir(t, td)()
+
+			ui := new(cli.MockUi)
+			c := &RefreshCommand{
+				Meta: Meta{
+					Ui: ui,
+				},
+			}
+
+			args := []string{
+				"-target", target,
+			}
+			if code := c.Run(args); code != 1 {
+				t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
+			}
+
+			got := ui.ErrorWriter.String()
+			if !strings.Contains(got, target) {
+				t.Fatalf("bad error output, want %q, got:\n%s", target, got)
+			}
+			if !strings.Contains(got, wantDiag) {
+				t.Fatalf("bad error output, want %q, got:\n%s", wantDiag, got)
+			}
+		})
+	}
+}
+
 // configuration in testdata/refresh . This schema should be
 // assigned to a mock provider named "test".
 func refreshFixtureSchema() *providers.GetSchemaResponse {

command/testdata/apply-destroy-targeted/main.tf

@@ -3,5 +3,5 @@ resource "test_instance" "foo" {
 }
 
 resource "test_load_balancer" "foo" {
-  instances = ["${test_instance.foo.*.id}"]
+  instances = test_instance.foo.*.id
 }

command/testdata/apply-targeted/main.tf

@@ -0,0 +1,9 @@
+resource "test_instance" "foo" {
+  count = 2
+}
+
+resource "test_instance" "bar" {
+}
+
+resource "test_instance" "baz" {
+}

command/testdata/refresh-targeted/main.tf

@@ -0,0 +1,7 @@
+resource "test_instance" "foo" {
+  id = "foo"
+}
+
+resource "test_instance" "bar" {
+  id = "bar"
+}