Skip to content

Add ValidateListResourceConfig RPC for List #310

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

Merged
merged 2 commits into from
Jun 12, 2025
Merged

Add ValidateListResourceConfig RPC for List #310

merged 2 commits into from
Jun 12, 2025

Conversation

rainkwan
Copy link
Contributor

@rainkwan rainkwan commented Jun 5, 2025

Related Issue

Fixes #

Description

Adds ValidateListResourceConfig RPC from List

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

No

@rainkwan rainkwan changed the title Started adding ValidateListResourceConfig Add ValidateListResourceConfig RPC for List Jun 5, 2025
@rainkwan rainkwan marked this pull request as ready for review June 5, 2025 17:23
@rainkwan rainkwan requested a review from a team as a code owner June 5, 2025 17:23
Copy link
Contributor

@bbasata bbasata left a comment

Choose a reason for hiding this comment

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

:shipit: Looks good to me. These review comments are non-blocking. Feel free to resolve in this PR or in a follow-up PR.

There's one correctness bit in the review comments on tfprotov5to6.go and tfprotov6to5.go.

@@ -202,7 +240,8 @@ func (s *muxServer) getResourceServer(ctx context.Context, typeName string) (tfp
// serverDiscovery will populate the mux server "routing" for functions and
// resource types by calling all underlying server GetMetadata RPC and falling
// back to GetProviderSchema RPC. It is intended to only be called through
// getDataSourceServer, getEphemeralResourceServer, getFunctionServer, and getResourceServer.
// getDataSourceServer, getEphemeralResourceServer, getListResourceServer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch 😃

}

ctx = logging.Tfprotov5ProviderServerContext(ctx, server)
logging.MuxTrace(ctx, "calling downstream server")
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that we do not log completion of the downstream server call. No action required here – this is consistent with other RPCs in mux. Just curious 💭 if that's useful.

//nolint:staticcheck // Intentionally verifying interface implementation
listResourceServer, ok := v6server.(tfprotov6.ProviderServerWithListResource)
if !ok {
t.Fatal("v6server should implement tfprotov6.ProviderServerWithResourceIdentity")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
t.Fatal("v6server should implement tfprotov6.ProviderServerWithResourceIdentity")
t.Fatal("v6server should implement tfprotov6.ProviderServerWithListResource")

@rainkwan rainkwan merged commit fcfffa0 into main Jun 12, 2025
34 checks passed
@rainkwan rainkwan deleted the rk/list-rpcs branch June 12, 2025 16:39
@rainkwan rainkwan added this to the v0.21.0 milestone Jun 13, 2025
rainkwan added a commit that referenced this pull request Jun 16, 2025
* Addressed comments in previous PR #310
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants