helper/resource: plannable import re-work to support resources with dependencies

[bbasata]

This change re-works plannable import testing to support a scenario uncovered in alpha releases: the resource-to-be-imported requires replacement ... because it depends on a resource that does not exist in state before the plan is generated.

The new approach: a plannable import TestStep constructs this state: [copy of state from previous config mode TestStep] minus [the "resource under test," indicated by TestStep.ResourceName].

terraform-plugin-testing/helper/resource/testing_new_import_state.go

Lines 155 to 169 in 88a9bc9

	if kind.plannable() {
	if stepNumber > 1 {
	err = importWd.CopyState(ctx, wd.StateFilePath())
	if err != nil {
	t.Fatalf("copying state: %s", err)
	}
	err = runProviderCommand(ctx, t, func() error {
	return importWd.RemoveResource(ctx, resourceName)
	}, importWd, providers)
	if err != nil {
	t.Fatalf("removing resource %s from copied state: %s", resourceName, err)
	}
	}
	}

The plan is generated against this prior state; therefore, only the the "resource under test" should have planned changes.

terraform-plugin-testing/helper/resource/testing_new_import_state.go

Lines 205 to 229 in 88a9bc9

	if len(plan.ResourceChanges) == 0 {
	return fmt.Errorf("importing resource %s: expected a resource change, got no changes", resourceName)
	}
	for _, change := range plan.ResourceChanges {
	if change.Address == resourceName {
	resourceChangeUnderTest = change
	}
	}
	if resourceChangeUnderTest == nil || resourceChangeUnderTest.Change == nil || resourceChangeUnderTest.Change.Actions == nil {
	return fmt.Errorf("importing resource %s: expected a resource change, got no changes", resourceName)
	}
	change := resourceChangeUnderTest.Change
	actions := change.Actions
	importing := change.Importing
	switch {
	case importing == nil:
	return fmt.Errorf("importing resource %s: expected an import operation, got %q action with plan \nstdout:\n\n%s", resourceChangeUnderTest.Address, actions, savedPlanRawStdout(ctx, t, importWd, providers))
	case !actions.NoOp():
	return fmt.Errorf("importing resource %s: expected a no-op import operation, got %q action with plan \nstdout:\n\n%s", resourceChangeUnderTest.Address, actions, savedPlanRawStdout(ctx, t, importWd, providers))
	}

This PR also makes a couple changes to turn off anything that expects plannable import to produce a new state. A "plan-only" plannable import has sufficed so far.

• Error on use of ImportStatePersist with plannable import

terraform-plugin-testing/helper/resource/testing_new_import_state.go

Lines 430 to 431 in 88a9bc9

	case kind.plannable() && step.ImportStatePersist:
	return fmt.Errorf(`ImportStatePersist is not supported with plannable import blocks`)

• Error on use of ImportStateVerify with plannable import
  • I was puzzled that ImportStateVerify was error-free for plannable import tests, despite the plan-only mechanics of these tests (no apply, no new state). I found that these tests trivially satisfied ImportStateVerify -- a loop over an empty newResources slice does nothing 😃

terraform-plugin-testing/helper/resource/testing_new_import_state.go

Lines 433 to 434 in 88a9bc9

	case kind.plannable() && step.ImportStateVerify:
	return fmt.Errorf(`ImportStateVerify is not supported with plannable import blocks`)

cc: @austinvalle who paired with me on 💭 and design
cc: @gdavison who raised the zone-domain example

[bbasata]

notes to self

[austinvalle]

Looks great! I left a couple nits/thoughts 🚀

[austinvalle]

This is less of a nit and more of a discussion our team should probably have at some point.

I'm not really sure how to do the changelogs for alpha/beta releases. I've currently been doing just one or two NOTE changelogs mentioning the general features and not specific deltas between releases, but I can see why you'd want to be more explicit in-case anyone was trying the features out 🤔

[austinvalle]

Also, I failed to call out the import test changes in the first changelog for alpha, so maybe a general note would be useful.

[bbasata]

This is less of a nit and more of a discussion our team should probably have at some point.

💯

I can see why you'd want to be more explicit in-case anyone was trying the features out 🤔

Right, I'm thinking to bias toward clarity here. Pre-releases will have partial implementations, so a little expectation-setting can go a long way.

[austinvalle]

This shouldn't block the PR merging, just some thinking out loud

I'm trying to figure out if this is all we need to do 🤔. I think in most cases this should suffice, but wondering if we have other external providers included in the first step... are we going to need to copy more (lockfile, downloaded providers)? Maybe the entire directory?

Would the follow-up init handle this seamlessly anyways? I think the state rm command should be safe since it doesn't need to initialize anything.

Maybe an eventual new test case could prove this theory out using a utility provider.

[bbasata]

+1 for a test case with external providers. I can add this in a follow-up PR.

[bbasata]

Would the follow-up init handle this seamlessly anyways?

I ran a quick test of this. Observations:

• Contents of a work directory

$ find step_1/work1565457440 -type f
step_1/work1565457440/testdata/1/examplecloud_container.tf
step_1/work1565457440/testdata/2/examplecloud_container_import.tf
step_1/work1565457440/terraform.tfstate
step_1/work1565457440/tfplan
step_1/work1565457440/.terraform/providers/registry.terraform.io/hashicorp/random/3.7.1/darwin_arm64/terraform-provider-random_v3.7.1_x5
step_1/work1565457440/.terraform/providers/registry.terraform.io/hashicorp/random/3.7.1/darwin_arm64/LICENSE.txt
step_1/work1565457440/terraform_plugin_test.tf
step_1/work1565457440/.terraform.lock.hcl

• Yes, the follow-up init handles this seamlessly
• The follow-up external provider download is an optimization opportunity -- why download twice? 😃
• What if ImportState used Terraform workspaces?

[austinvalle]

It just now struck me reading this code that a resource change will appear even if it's a no-op 😆

nit - Maybe this error messaging could reflect what we actually mean, i.e, the resource was not found in the plan at all (change != planned to be updated)

[bbasata]

👍 to a more specific error message. Technically, this is simply checking safety of traversing plan.ResourceChanges, so it's the means to checking what we want. So we can make the test feedback more about the "end" than the "means."