chore: Fix identity TODOs and add documentation for interfaces (#1133)

* chore: Fix identity TODOs and add documentation for interfaces

* Update resource/resource.go

Author: austinvalle

internal/fromproto5/resource_identity.go

@@ -13,9 +13,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-go/tfprotov5"
 )
 
-// TODO:ResourceIdentity: Should we create a wrapping struct to contain the identity data? To match the protocol (in-case we want to introduce other identity things)
-// - Need to think more on this (like what if we want to introduce display-only attributes)
-// - If we introduce one, add a test as well.
+// ResourceIdentity returns the *tfsdk.ResourceIdentity for a *tfprotov5.ResourceIdentityData and fwschema.Schema.
 func ResourceIdentity(ctx context.Context, in *tfprotov5.ResourceIdentityData, schema fwschema.Schema) (*tfsdk.ResourceIdentity, diag.Diagnostics) {
 	if in == nil {
 		return nil, nil

internal/fromproto6/resource_identity.go

@@ -13,9 +13,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-go/tfprotov6"
 )
 
-// TODO:ResourceIdentity: Should we create a wrapping struct to contain the identity data? To match the protocol (in-case we want to introduce other identity things)
-// - Need to think more on this (like what if we want to introduce display-only attributes)
-// - If we introduce one, add a test as well.
+// ResourceIdentity returns the *tfsdk.ResourceIdentity for a *tfprotov6.ResourceIdentityData and fwschema.Schema.
 func ResourceIdentity(ctx context.Context, in *tfprotov6.ResourceIdentityData, schema fwschema.Schema) (*tfsdk.ResourceIdentity, diag.Diagnostics) {
 	if in == nil {
 		return nil, nil

internal/fwserver/server_createresource.go

@@ -101,7 +101,6 @@ func (s *Server) CreateResource(ctx context.Context, req *CreateResourceRequest,
 	}
 
 	// If the resource supports identity and there is no planned identity data, pre-populate with a null value.
-	// TODO:ResourceIdentity: This logic is likely useless since plan should already handle this, probably should remove.
 	if req.PlannedIdentity == nil && req.IdentitySchema != nil {
 		nullIdentityTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil)
 

internal/fwserver/server_deleteresource.go

@@ -98,8 +98,6 @@ func (s *Server) DeleteResource(ctx context.Context, req *DeleteResourceRequest,
 		resp.Private = req.PlannedPrivate
 	}
 
-	// If the resource supports identity pre-populate a null value.
-	// TODO:ResourceIdentity: This should probably be prior identity, but we don't currently have that in the protocol.
 	if req.IdentitySchema != nil {
 		nullIdentityTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil)
 

internal/fwserver/server_importresourcestate.go

@@ -149,7 +149,6 @@ func (s *Server) ImportResourceState(ctx context.Context, req *ImportResourceSta
 	}
 
 	// If the resource supports identity and we are not importing by identity, pre-populate with a null value.
-	// TODO:ResourceIdentity: Is there any reason a provider WOULD NOT want to populate an identity when it supports one?
 	if req.Identity == nil && req.IdentitySchema != nil {
 		nullTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil)
 

internal/fwserver/server_planresourcechange.go

@@ -119,8 +119,6 @@ func (s *Server) PlanResourceChange(ctx context.Context, req *PlanResourceChange
 	}
 
 	// If the resource supports identity and there is no prior identity data, pre-populate with a null value.
-	// TODO:ResourceIdentity: Is there any reason a provider WOULD NOT want to populate an identity when it supports one?
-	// TODO:ResourceIdentity: Should this be set to all unknowns?
 	if req.PriorIdentity == nil && req.IdentitySchema != nil {
 		nullIdentityTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil)
 

internal/fwserver/server_readresource.go

@@ -120,7 +120,6 @@ func (s *Server) ReadResource(ctx context.Context, req *ReadResourceRequest, res
 	}
 
 	// If the resource supports identity and there is no current identity data, pre-populate with a null value.
-	// TODO:ResourceIdentity: Is there any reason a provider WOULD NOT want to populate an identity when it supports one?
 	if req.CurrentIdentity == nil && req.IdentitySchema != nil {
 		nullTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil)
 

internal/fwserver/server_updateresource.go

@@ -122,7 +122,6 @@ func (s *Server) UpdateResource(ctx context.Context, req *UpdateResourceRequest,
 	}
 
 	// If the resource supports identity and there is no planned identity data, pre-populate with a null value.
-	// TODO:ResourceIdentity: This logic is likely useless since plan should already handle this, probably should remove.
 	if req.PlannedIdentity == nil && req.IdentitySchema != nil {
 		nullIdentityTfValue := tftypes.NewValue(req.IdentitySchema.Type().TerraformType(ctx), nil)
 

resource/resource.go

@@ -19,6 +19,9 @@ import (
 //   - Plan Modification: Schema-based or entire plan
 //     via ResourceWithModifyPlan.
 //   - State Upgrades: ResourceWithUpgradeState
+//   - Identity: Define an identity schema with ResourceWithIdentity to enable
+//     storing identity data in state during CRUD operations. Identity data is
+//     used by Terraform to uniquely identify a managed resource.
 //
 // Although not required, it is conventional for resources to implement the
 // ResourceWithImportState interface.
@@ -199,7 +202,21 @@ type ResourceWithValidateConfig interface {
 
 // ResourceWithIdentity is an interface type that extends Resource to implement managed resource identity.
 //
-// TODO:ResourceIdentity: Add more documentation here to describe what identity is used for.
+// Managed resources can optionally define an identity schema, which represents a separate object stored in state
+// alongside the resource instance data. This identity data is used by Terraform to uniquely identify
+// managed resources and has additional restrictions that allow external programs to determine equality
+// between two identities.
+//
+// Resource identity schemas can only contain primitive (bool, string, number) attributes and lists that
+// contain primitive elements. Additionally, a resource identity should have the following properties:
+//   - The resource identity should correspond to at most one remote object per provider, across all
+//     instances of that provider.
+//   - Given a resource identity, the provider should be able to determine whether the corresponding remote
+//     object exists, and if so, return the resource state. Resources that support identity can be imported
+//     by the identity object via the ResourceWithImportState interface.
+//   - The resource identity should not change during the lifecycle of the remote object. That is, from the
+//     creation of the remote object in the remote system until its destruction. An exception to this rule
+//     is an upgrade of the identity data after a schema change, via the ResourceWithUpgradeIdentity interface.
 type ResourceWithIdentity interface {
 	Resource