-
Notifications
You must be signed in to change notification settings - Fork 11
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.Fatal("v6server should implement tfprotov6.ProviderServerWithResourceIdentity") | |
t.Fatal("v6server should implement tfprotov6.ProviderServerWithListResource") |
Related Issue
Fixes #
Description
Adds ValidateListResourceConfig RPC from List
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
No