tfsdk: Initial ImportResourceState support

[bflad]

Closes #33

[paddycarver]

Looks great to me, some small quibbles.

[paddycarver]

Why do all of the other test resource types get implementations that update importResourceStateCalledResourceType, but the validate ones don't?

[bflad]

Ah, this is because when I implemented those two test resources I was trying to omit parts that would be uncalled so future travelers just focused on the importantly implemented methods. It should have been commented as such.

Do you have a preference between these options?

1. Add a comment in these implementations, e.g. // Intentionally blank. Not expected to be called during testing.
2. Copy the implementations of other test resources (recognizing they should all stay in sync going forward; not sure how to enforce that)
3. Removing other unexpected test resource implementations, doing something like option 1 so it is more clear

Thanks!

[paddycarver]

I think my preference is 3, but replacing them with our not supported function and a comment saying that functionality is tested elsewhere. What do you think?

I mostly just am :/ about having code we don't need in the test resources if we're not testing it, because that breeds confusion about whether it's used and where and what can be safely changed and what can't be in the future, I think. 🤔

[bflad]

Agreed! That will work for the new ImportState method implementations and I'll add the comment to the other Create/Read/Update/Delete test implementations that should be empty. Will update shortly after merging main to ensure this is ready to rock. 🎸

[bflad]

Everything should be ready for review again.

Something that gives me pause at the moment is how tooling will determine if a resource actually supports import and potentially how it can signal an example import identifier for documentation generation. I'm not sure if that should necessarily block an initial implementation, but it might be something worth iterating on in the future. Would love to hear thoughts and/or if I should create a followup issue.

[bflad]

Created #160 for potential followup.

[paddycarver]

Yeah, let's go ahead and do it. 👍 🚀 Nice work.

[kmoe]

LGTM

[paddycarver]

A thing worth thinking about here:

We need a resolution to #148 to make this work the way we want.

Another option is to have it do something like:

vals := map[string]tftypes.Value{}
typ := resourceSchema.TerraformType(ctx)
for name, t := range typ.(tftypes.Object).AttributeTypes {
  vals[name] = tftypes.NewValue(t, nil)
}
emptyState := tftypes.NewValue(typ, vals)

This matches the more-expected (imho) default state, which is an object in state with nothing filled in, instead of nothing in state.

However, this is weird during failure cases. I'm not clear on whether Terraform will persist the object to state or write nothing to state in the face of an error diagnostic. If the former, we should default to an empty state and just rely on #148. If the latter, we should confirm that will continue to be the case, and maybe defaulting to the empty object (instead of the null object) is more expected? I dunno. I don't know that it makes a huge difference, just wanted to call it out here.

[bflad]

The current logic in Terraform CLI is to return early on ImportResourceState error diagnostics, before saving state information:

https://github.com/hashicorp/terraform/blob/2afa0a5e75d76de7e47157114c579b3b1bff994f/internal/terraform/transform_import_state.go#L141-L154

[paddycarver]

🤔 how do we feel about just leaving it and waiting for feedback?

[bflad]

We're going to merge this in as-is for now and iterate if necessary.

If we do wind up switching the implementation here in the future -- maybe creating the above code snippet as a method such as (Schema).newState() would be good to try and prevent "empty" object problems here and in any other potential places that might need it. That might be something we could export if/when multiple resource import support makes sense.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.