Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add resource identity to Move RPC #1123

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

rainkwan
Copy link

@rainkwan rainkwan commented Mar 27, 2025

Follow-Up PR for #1112

This PR implements MoveResourceState (Move) for identity.

@rainkwan rainkwan marked this pull request as ready for review April 2, 2025 18:43
@rainkwan rainkwan requested a review from a team as a code owner April 2, 2025 18:43
@rainkwan rainkwan added the enhancement New feature or request label Apr 2, 2025
@rainkwan rainkwan added this to the v1.15.0 milestone Apr 2, 2025
@rainkwan rainkwan force-pushed the rk/resource-identity branch from 718b775 to f3504cb Compare April 14, 2025 15:33
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall everything is looking good! I had a couple questions/nits

Comment on lines +166 to +189
"SourceIdentity": {
input: &tfprotov5.MoveResourceStateRequest{
SourceIdentity: testNewTfprotov5RawState(t, map[string]interface{}{
"test_identity_attribute": "test-value",
}),
},
resourceSchema: testFwSchema,
expected: &fwserver.MoveResourceStateRequest{
SourceIdentity: testNewTfprotov6RawState(t, map[string]interface{}{
"test_identity_attribute": "test-value",
}),
TargetResourceSchema: testFwSchema,
},
},
"SourceIdentitySchemaVersion": {
input: &tfprotov5.MoveResourceStateRequest{
SourceIdentitySchemaVersion: 123,
},
resourceSchema: testFwSchema,
expected: &fwserver.MoveResourceStateRequest{
SourceIdentitySchemaVersion: 123,
TargetResourceSchema: testFwSchema,
},
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - Neither of these tests use an identitySchema, which is technically valid for this unit test, but not valid in general 😄 (same note for the fromproto6 tests)

Example:

  • testIdentitySchema := identityschema.Schema{
    Attributes: map[string]identityschema.Attribute{
    "test_identity_attribute": identityschema.StringAttribute{
    RequiredForImport: true,
    },
    },
    }
  • "identity": {
    input: &tfprotov5.ImportResourceStateRequest{
    Identity: &tfprotov5.ResourceIdentityData{
    IdentityData: &testIdentityProto5DynamicValue,
    },
    },
    resourceSchema: testFwSchema,
    identitySchema: testIdentitySchema,
    expected: &fwserver.ImportResourceStateRequest{
    EmptyState: testFwEmptyState,
    IdentitySchema: testIdentitySchema,
    Identity: &tfsdk.ResourceIdentity{
    Raw: testIdentityProto5Value,
    Schema: testIdentitySchema,
    },
    },
    },

Comment on lines +274 to +275
"Target Resource Type: "+req.TargetTypeName, //+"\n"+
// "Source Identity Schema Version: "+strconv.FormatInt(req.SourceSchemaVersion, 10),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you meaning to update this diagnostic with the source identity information? If so, you'll just need to make sure you only print something if we actually receive a source identity.

I think core will always store 0 for the identity schema version in state regardless of it the resource supports identity, so we might want to check SourceIdentity for that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - Since we have the logic checking for an invalid identity response, we should consider adding a small test for it 👍🏻

Example:

"response-invalid-identity": {
server: &fwserver.Server{
Provider: &testprovider.Provider{},
},
request: &fwserver.ReadResourceRequest{
CurrentState: testCurrentState,
Resource: &testprovider.Resource{
ReadMethod: func(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
// This resource doesn't indicate identity support (via a schema), so this should raise a diagnostic.
resp.Identity = &tfsdk.ResourceIdentity{
Raw: testNewIdentityValue,
Schema: testIdentitySchema,
}
},
},
},
expectedResponse: &fwserver.ReadResourceResponse{
Diagnostics: diag.Diagnostics{
diag.NewErrorDiagnostic(
"Unexpected Read Response",
"An unexpected error was encountered when creating the read response. New identity data was returned by the provider read operation, but the resource does not indicate identity support.\n\n"+
"This is always a problem with the provider and should be reported to the provider developer.",
),
},
NewState: testCurrentState,
NewIdentity: testNewIdentity,
Private: testEmptyPrivate,
},
},

@@ -14,7 +14,7 @@ import (

// ResourceIdentity returns the *tfprotov5.ResourceIdentityData for a *tfsdk.ResourceIdentity.
func ResourceIdentity(ctx context.Context, fw *tfsdk.ResourceIdentity) (*tfprotov5.ResourceIdentityData, diag.Diagnostics) {
if fw == nil {
if fw == nil || fw.Schema == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something you ran into specifically? I would expect this to never happen 🤔

// Only the underlying JSON field is populated.
SourceIdentity *tfprotov6.RawState

// SourceIdentitySchemaVersion is the version of the source resource state.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// SourceIdentitySchemaVersion is the version of the source resource state.
// SourceIdentitySchemaVersion is the version of the source resource identity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants