terraform: refactor graphNodeImportState and graphNodeImportState (#26243)

EvalTrees

The import EvalTree()s have been replaced with Execute +
straight-through code, which let me remove eval_import_state.go.

graphNodeImportStateSub's Execute() function is still using the
old-style (not-refactored) calls to EvalRefresh and EvalWriteState;
those are being saved for a later refactor once all (or at least most)
of the EvalTree()s are gone.

I've added incredibly basic tests for both Execute() functions; they are
only enough to verify basics - the real testing happens in
context_import_test.go.

terraform/eval_import_state.go

@@ -1,95 +0,0 @@
-package terraform
-
-import (
-	"fmt"
-	"log"
-
-	"github.com/hashicorp/terraform/addrs"
-	"github.com/hashicorp/terraform/providers"
-	"github.com/hashicorp/terraform/states"
-	"github.com/hashicorp/terraform/tfdiags"
-)
-
-// EvalImportState is an EvalNode implementation that performs an
-// ImportState operation on a provider. This will return the imported
-// states but won't modify any actual state.
-type EvalImportState struct {
-	Addr     addrs.ResourceInstance
-	Provider *providers.Interface
-	ID       string
-	Output   *[]providers.ImportedResource
-}
-
-// TODO: test
-func (n *EvalImportState) Eval(ctx EvalContext) (interface{}, error) {
-	absAddr := n.Addr.Absolute(ctx.Path())
-	provider := *n.Provider
-	var diags tfdiags.Diagnostics
-
-	{
-		// Call pre-import hook
-		err := ctx.Hook(func(h Hook) (HookAction, error) {
-			return h.PreImportState(absAddr, n.ID)
-		})
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	resp := provider.ImportResourceState(providers.ImportResourceStateRequest{
-		TypeName: n.Addr.Resource.Type,
-		ID:       n.ID,
-	})
-	diags = diags.Append(resp.Diagnostics)
-	if diags.HasErrors() {
-		return nil, diags.Err()
-	}
-
-	imported := resp.ImportedResources
-
-	for _, obj := range imported {
-		log.Printf("[TRACE] EvalImportState: import %s %q produced instance object of type %s", absAddr.String(), n.ID, obj.TypeName)
-	}
-
-	if n.Output != nil {
-		*n.Output = imported
-	}
-
-	{
-		// Call post-import hook
-		err := ctx.Hook(func(h Hook) (HookAction, error) {
-			return h.PostImportState(absAddr, imported)
-		})
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	return nil, nil
-}
-
-// EvalImportStateVerify verifies the state after ImportState and
-// after the refresh to make sure it is non-nil and valid.
-type EvalImportStateVerify struct {
-	Addr  addrs.ResourceInstance
-	State **states.ResourceInstanceObject
-}
-
-// TODO: test
-func (n *EvalImportStateVerify) Eval(ctx EvalContext) (interface{}, error) {
-	var diags tfdiags.Diagnostics
-
-	state := *n.State
-	if state.Value.IsNull() {
-		diags = diags.Append(tfdiags.Sourceless(
-			tfdiags.Error,
-			"Cannot import non-existent remote object",
-			fmt.Sprintf(
-				"While attempting to import an existing object to %s, the provider detected that no object exists with the given id. Only pre-existing objects can be imported; check that the id is correct and that it is associated with the provider's configured region or endpoint, or use \"terraform apply\" to create a new remote object for this resource.",
-				n.Addr.String(),
-			),
-		))
-	}
-
-	return nil, diags.ErrWithWarnings()
-}

terraform/transform_import_state.go

@@ -2,6 +2,7 @@ package terraform
 
 import (
 	"fmt"
+	"log"
 
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/configs"
@@ -70,7 +71,7 @@ type graphNodeImportState struct {
 
 var (
 	_ GraphNodeModulePath        = (*graphNodeImportState)(nil)
-	_ GraphNodeEvalable          = (*graphNodeImportState)(nil)
+	_ GraphNodeExecutable        = (*graphNodeImportState)(nil)
 	_ GraphNodeProviderConsumer  = (*graphNodeImportState)(nil)
 	_ GraphNodeDynamicExpandable = (*graphNodeImportState)(nil)
 )
@@ -114,28 +115,48 @@ func (n *graphNodeImportState) ModulePath() addrs.Module {
 	return n.Addr.Module.Module()
 }
 
-// GraphNodeEvalable impl.
+// GraphNodeExecutable impl.
-func (n *graphNodeImportState) EvalTree() EvalNode {
+func (n *graphNodeImportState) Execute(ctx EvalContext, op walkOperation) error {
-	var provider providers.Interface
-
 	// Reset our states
 	n.states = nil
 
-	// Return our sequence
+	provider, _, err := GetProvider(ctx, n.ResolvedProvider)
-	return &EvalSequence{
+	if err != nil {
-		Nodes: []EvalNode{
+		return err
-			&EvalGetProvider{
-				Addr:   n.ResolvedProvider,
-				Output: &provider,
-			},
-			&EvalImportState{
-				Addr:     n.Addr.Resource,
-				Provider: &provider,
-				ID:       n.ID,
-				Output:   &n.states,
-			},
-		},
 	}
+
+	// import state
+	absAddr := n.Addr.Resource.Absolute(ctx.Path())
+	var diags tfdiags.Diagnostics
+
+	// Call pre-import hook
+	err = ctx.Hook(func(h Hook) (HookAction, error) {
+		return h.PreImportState(absAddr, n.ID)
+	})
+	if err != nil {
+		return err
+	}
+
+	resp := provider.ImportResourceState(providers.ImportResourceStateRequest{
+		TypeName: n.Addr.Resource.Resource.Type,
+		ID:       n.ID,
+	})
+	diags = diags.Append(resp.Diagnostics)
+	if diags.HasErrors() {
+		return diags.Err()
+	}
+
+	imported := resp.ImportedResources
+	for _, obj := range imported {
+		log.Printf("[TRACE] graphNodeImportState: import %s %q produced instance object of type %s", absAddr.String(), n.ID, obj.TypeName)
+	}
+	n.states = imported
+
+	// Call post-import hook
+	err = ctx.Hook(func(h Hook) (HookAction, error) {
+		return h.PostImportState(absAddr, imported)
+	})
+	return err
 }
 
 // GraphNodeDynamicExpandable impl.
@@ -194,9 +215,9 @@ func (n *graphNodeImportState) DynamicExpand(ctx EvalContext) (*Graph, error) {
 	}
 
 	// For each of the states, we add a node to handle the refresh/add to state.
-	// "n.states" is populated by our own EvalTree with the result of
+	// "n.states" is populated by our own Execute with the result of
-	// ImportState. Since DynamicExpand is always called after EvalTree, this
+	// ImportState. Since DynamicExpand is always called after Execute, this is
-	// is safe.
+	// safe.
 	for i, state := range n.states {
 		g.Add(&graphNodeImportStateSub{
 			TargetAddr:       addrs[i],
@@ -226,7 +247,7 @@ type graphNodeImportStateSub struct {
 
 var (
 	_ GraphNodeModuleInstance = (*graphNodeImportStateSub)(nil)
-	_ GraphNodeEvalable       = (*graphNodeImportStateSub)(nil)
+	_ GraphNodeExecutable     = (*graphNodeImportStateSub)(nil)
 )
 
 func (n *graphNodeImportStateSub) Name() string {
@@ -237,43 +258,54 @@ func (n *graphNodeImportStateSub) Path() addrs.ModuleInstance {
 	return n.TargetAddr.Module
 }
 
-// GraphNodeEvalable impl.
+// GraphNodeExecutable impl.
-func (n *graphNodeImportStateSub) EvalTree() EvalNode {
+func (n *graphNodeImportStateSub) Execute(ctx EvalContext, op walkOperation) error {
 	// If the Ephemeral type isn't set, then it is an error
 	if n.State.TypeName == "" {
-		err := fmt.Errorf("import of %s didn't set type", n.TargetAddr.String())
+		return fmt.Errorf("import of %s didn't set type", n.TargetAddr.String())
-		return &EvalReturnError{Error: &err}
 	}
 
 	state := n.State.AsInstanceObject()
-
+	provider, providerSchema, err := GetProvider(ctx, n.ResolvedProvider)
-	var provider providers.Interface
+	if err != nil {
-	var providerSchema *ProviderSchema
+		return err
-	return &EvalSequence{
-		Nodes: []EvalNode{
-			&EvalGetProvider{
-				Addr:   n.ResolvedProvider,
-				Output: &provider,
-				Schema: &providerSchema,
-			},
-			&EvalRefresh{
-				Addr:           n.TargetAddr.Resource,
-				ProviderAddr:   n.ResolvedProvider,
-				Provider:       &provider,
-				ProviderSchema: &providerSchema,
-				State:          &state,
-				Output:         &state,
-			},
-			&EvalImportStateVerify{
-				Addr:  n.TargetAddr.Resource,
-				State: &state,
-			},
-			&EvalWriteState{
-				Addr:           n.TargetAddr.Resource,
-				ProviderAddr:   n.ResolvedProvider,
-				ProviderSchema: &providerSchema,
-				State:          &state,
-			},
-		},
 	}
+
+	// EvalRefresh
+	evalRefresh := &EvalRefresh{
+		Addr:           n.TargetAddr.Resource,
+		ProviderAddr:   n.ResolvedProvider,
+		Provider:       &provider,
+		ProviderSchema: &providerSchema,
+		State:          &state,
+		Output:         &state,
+	}
+	_, err = evalRefresh.Eval(ctx)
+	if err != nil {
+		return err
+	}
+
+	// Verify the existance of the imported resource
+	if state.Value.IsNull() {
+		var diags tfdiags.Diagnostics
+		diags = diags.Append(tfdiags.Sourceless(
+			tfdiags.Error,
+			"Cannot import non-existent remote object",
+			fmt.Sprintf(
+				"While attempting to import an existing object to %s, the provider detected that no object exists with the given id. Only pre-existing objects can be imported; check that the id is correct and that it is associated with the provider's configured region or endpoint, or use \"terraform apply\" to create a new remote object for this resource.",
+				n.TargetAddr.Resource.String(),
+			),
+		))
+		return diags.Err()
+	}
+
+	//EvalWriteState
+	evalWriteState := &EvalWriteState{
+		Addr:           n.TargetAddr.Resource,
+		ProviderAddr:   n.ResolvedProvider,
+		ProviderSchema: &providerSchema,
+		State:          &state,
+	}
+	_, err = evalWriteState.Eval(ctx)
+	return err
 }

terraform/transform_import_state_test.go

@@ -0,0 +1,109 @@
+package terraform
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/hashicorp/terraform/addrs"
+	"github.com/hashicorp/terraform/configs/configschema"
+	"github.com/hashicorp/terraform/providers"
+	"github.com/hashicorp/terraform/states"
+	"github.com/zclconf/go-cty/cty"
+)
+
+func TestGraphNodeImportStateExecute(t *testing.T) {
+	state := states.NewState()
+	provider := testProvider("aws")
+	provider.ImportStateReturn = []*InstanceState{
+		&InstanceState{
+			ID:        "bar",
+			Ephemeral: EphemeralState{Type: "aws_instance"},
+		},
+	}
+	ctx := &MockEvalContext{
+		StateState:       state.SyncWrapper(),
+		ProviderProvider: provider,
+	}
+
+	// Import a new aws_instance.foo, this time with ID=bar. The original
+	// aws_instance.foo object should be removed from state and replaced with
+	// the new.
+	node := graphNodeImportState{
+		Addr: addrs.Resource{
+			Mode: addrs.ManagedResourceMode,
+			Type: "aws_instance",
+			Name: "foo",
+		}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
+		ID: "bar",
+		ResolvedProvider: addrs.AbsProviderConfig{
+			Provider: addrs.NewDefaultProvider("aws"),
+			Module:   addrs.RootModule,
+		},
+	}
+
+	err := node.Execute(ctx, walkImport)
+	if err != nil {
+		t.Fatalf("Unexpected error: %s", err.Error())
+	}
+
+	if len(node.states) != 1 {
+		t.Fatalf("Wrong result! Expected one imported resource, got %d", len(node.states))
+	}
+	// Verify the ID for good measure
+	id := node.states[0].State.GetAttr("id")
+	if !id.RawEquals(cty.StringVal("bar")) {
+		t.Fatalf("Wrong result! Expected id \"bar\", got %q", id.AsString())
+	}
+}
+
+func TestGraphNodeImportStateSubExecute(t *testing.T) {
+	state := states.NewState()
+	provider := testProvider("aws")
+	ctx := &MockEvalContext{
+		StateState:       state.SyncWrapper(),
+		ProviderProvider: provider,
+		ProviderSchemaSchema: &ProviderSchema{
+			ResourceTypes: map[string]*configschema.Block{
+				"aws_instance": {
+					Attributes: map[string]*configschema.Attribute{
+						"id": {
+							Type:     cty.String,
+							Computed: true,
+						},
+					},
+				},
+			},
+		},
+	}
+
+	importedResource := providers.ImportedResource{
+		TypeName: "aws_instance",
+		State:    cty.ObjectVal(map[string]cty.Value{"id": cty.StringVal("bar")}),
+	}
+
+	node := graphNodeImportStateSub{
+		TargetAddr: addrs.Resource{
+			Mode: addrs.ManagedResourceMode,
+			Type: "aws_instance",
+			Name: "foo",
+		}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
+		State: importedResource,
+		ResolvedProvider: addrs.AbsProviderConfig{
+			Provider: addrs.NewDefaultProvider("aws"),
+			Module:   addrs.RootModule,
+		},
+	}
+	err := node.Execute(ctx, walkImport)
+	if err != nil {
+		t.Fatalf("Unexpected error: %s", err.Error())
+	}
+
+	// check for resource in state
+	actual := strings.TrimSpace(state.String())
+	expected := `aws_instance.foo:
+  ID = bar
+  provider = provider["registry.terraform.io/hashicorp/aws"]`
+	if actual != expected {
+		t.Fatalf("bad state after import: \n%s", actual)
+	}
+}