Validate identity in import response

[dbanck]

This PR adds a check to ensure that the import RPC returns an identity when Terraform is doing an import by identity.

Target Release

1.12.x

CHANGELOG entry

• This change is user-facing and I added a changelog entry.
• This change is not user-facing.

[SarahFrench]

👋🏻 I'm dipping my toe into Resource Identity stuff as this PR looked manageable for a newbie.

At this point could the importType still be either "ID" or "Identity"? If Terraform was trying to import by ID, would the identity being null still be a problem here?

[SarahFrench]

Basically I'm wondering if the condition also needs importType == "Identity" in it

[dbanck]

We're doing the same check (importTarget.Type().IsObjectType()) further up to set the value of importType.

So checking importType == "Identity" is equal to checking importTarget.Type().IsObjectType()

[SarahFrench]

Ah yes I understand now, thanks! 😅

[dbanck]

Do you think changing importTarget.Type().IsObjectType() to importType == "Identity" would make it more readable in this case?

The sprinkled .IsObjectType() checks trough the codebase feel a bit odd to me

[SarahFrench]

As someone less familiar with the code I think it'd be clearer if there was something more explicit like importType used in conditional logic, versus the implicit meaning of having object-type or string data as the target.

Maybe the strings "ID" and "Identity" could be made into constants to help with any comparison expressions

[jbardin]

I'm not sure what would be more descriptive if you're unfamiliar with the code. The prior knowledge of knowing the difference in usage of a string vs an object isn't far from knowing the difference between ID and Identity. I'd rather not have multiple fields that need to be updated in concert, since that invites them to end up out of sync, so the other option would probably be a struct containing an enum of the possible types, providing a type declaration with appropriate comments. We already have an ImportTarget type which could be used, though that might have been dropped because it makes some other part of the expansion code simpler.

[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.