-
Notifications
You must be signed in to change notification settings - Fork 11
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
Ensure GetProviderSchema Response Error/Diagnostics are Unit Tested #177
Conversation
…agnostics in the GetProviderSchemaResponse (#176)
type TestServerDiags struct { | ||
*TestServer | ||
Diagnostics []*tfprotov5.Diagnostic | ||
} | ||
|
||
func (s *TestServerDiags) GetProviderSchema(ctx context.Context, req *tfprotov5.GetProviderSchemaRequest) (*tfprotov5.GetProviderSchemaResponse, error) { | ||
resp, err := s.TestServer.GetProviderSchema(ctx, req) | ||
|
||
resp.Diagnostics = append(resp.Diagnostics, s.Diagnostics...) | ||
|
||
return resp, err | ||
} |
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.
Instead of setting up a new test server type, how do you feel about updating the existing TestServer
to store a full GetProviderSchemaResponse *tfprotov5.GetProviderSchemaResponse
field similar to PrepareProviderConfigResponse *tfprotov5.PrepareProviderConfigResponse
?
We can migrate all the other GetProviderSchema
related fields to be under that as well, rather than individually calling them out:
DataSourceSchemas map[string]*tfprotov5.Schema
ProviderMetaSchema *tfprotov5.Schema
ProviderSchema *tfprotov5.Schema
ResourceSchemas map[string]*tfprotov5.Schema
ServerCapabilities *tfprotov5.ServerCapabilities
I'm going to peek at the other RPC logic and create an issue if it makes sense to do something similar for the other RPCs.
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.
Good call. Have refactored.
body: 'tf5muxserver: Include diagnostics returned from `server.GetProviderSchema()` | ||
in `muxServer.GetProviderSchemaResponse`' |
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.
Nit: I think we can tailor this message to developers without talking about the internal details, e.g.
tf5muxserver: Ensure GetProviderSchema
RPC diagnostics are properly returned to Terraform
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.
Makes sense, have updated.
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 great to me 🚀 Thank you for hunkering down to add the response object in there
I'll try and circle back to take of Ensure ConfigureProvider Response Diagnostics are Unit Tested in the near future. |
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. |
Closes: #176