-
Notifications
You must be signed in to change notification settings - Fork 238
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
tests: capture GRPC traffic also #1655
tests: capture GRPC traffic also #1655
Conversation
justinsb
commented
Apr 26, 2024
- tfprovider: support intercepting GRPC requests
- tests: record grpc requests in our golden tests
- tests: capture golden output for bigtabletable test
712924c
to
87c076f
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.
This probably needs to be called something else then, right? Like _grpc.golden.log
?
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.
maybe ... the issue is that it's still http traffic, we're just decoding the GRPC requests. There are some http logs where we intermingle http and GRPC!
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.
We could maybe rename _http.log
to _requests.log
or _gcp.log
- it's really the log of non-kube requests/responses.
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 think keeping the name as it is should be good. From the testing level, it doesn't really matter where the resource comes from or talks to. We care more on the resource itself.
@@ -32,6 +33,10 @@ func (s BigtableClientFactory) NewInstanceAdminClient(project string) (*bigtable | |||
opts = append(opts, option.WithTokenSource(s.TokenSource), option.WithUserAgent(s.UserAgent)) | |||
opts = append(opts, s.gRPCLoggingOptions...) | |||
|
|||
if GRPCUnaryClientInterceptor != 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.
Is this change from the upstream tf repo fetch or we manually added the logic?
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.
Added manually
/lgtm The fail test looks suspicious. Could you rerun? |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
We aim to construct a readable version of the GRPC requests.
This uses our new GRPC collection infrastructure.
/lgtm |
040ef44
into
GoogleCloudPlatform:master