-
Notifications
You must be signed in to change notification settings - Fork 9.9k
Testing list blocks with the gRPC Provider #37236
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
f3a42db
to
fcd9e12
Compare
fcd9e12
to
0e194dd
Compare
0e194dd
to
572aac4
Compare
572aac4
to
c5b213a
Compare
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'm really unsure about the introduction of grpcwrap in the internal/terraform package. It feels like grcpwrap is only used for e2e tests and the simple providers so far. Maybe we can Zoom about this tomorrow?
if p.GetProviderSchemaResponse != nil { | ||
|
||
for typeName, schema := range p.GetProviderSchemaResponse.ResourceTypes { | ||
if schema.Identity != nil { |
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.
Usually the resource identity schemas come from the GetResourceIdentitySchemas
call and not the other way around. I feel like extracting it from the ResourceTypes could lead to wrong assumptions
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.
This provider is a little different, in that it conforms to the internal providers.Provider
interface rather than the protobuf interface. That means the schemas are already going to be coalesced into a single object. That's not to say that this may still be missing some edge case, just saying that it's not necessarily wrong to assume they are one and the same at this point.
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.
Also, this is only a fallback when you do not provide a separate resource identity response.
A followup e2e test would wire up both layers
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.
🚢
Fixes #
Target Release
1.13.x
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry