feat(cloudrun): update sample service for cloud run readiness probes to support grpc #5404
Conversation
product-auto-label
bot
added
the
samples
Summary of ChangesHello @neilalberg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Cloud Run readiness probe sample by integrating gRPC health check capabilities. The changes allow the sample service to demonstrate how to expose and respond to gRPC readiness probes, providing a more robust and versatile example for developers deploying applications with gRPC services on Cloud Run. This ensures the sample remains relevant for modern microservice architectures that often leverage gRPC for inter-service communication and health monitoring. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for gRPC readiness probes to the Cloud Run sample service. The changes include updating the service to start a gRPC health server, modifying the data structures to handle gRPC probe configurations, and adding a new end-to-end test for the gRPC probe functionality.
The implementation is solid, but I have a few suggestions to improve code clarity, maintainability, and fix a minor UI issue. My feedback includes refactoring duplicated test code, removing an unused method, and improving the gRPC server implementation by leveraging UnimplementedHealthServer for forward compatibility.
| func TestServiceHealthGrpc(t *testing.T) { | ||
| tc := testutil.EndToEndTest(t) | ||
|
|
||
| service := cloudrunci.NewService("service-health", tc.ProjectID) | ||
| service.Readiness = &cloudrunci.ReadinessProbe{ | ||
| TimeoutSeconds: 1, | ||
| PeriodSeconds: 1, | ||
| SuccessThreshold: 1, | ||
| FailureThreshold: 1, | ||
| GRPC: &cloudrunci.GRPCProbe{ | ||
| Port: 8081, | ||
| }, | ||
| } | ||
| service.Dir = "../service-health" | ||
| service.AsBuildpack = true | ||
| service.Platform.CommandFlags() | ||
|
|
||
| if err := service.Deploy(); err != nil { | ||
| t.Fatalf("service.Deploy %q: %v", service.Name, err) | ||
| } | ||
| defer func(service *cloudrunci.Service) { | ||
| err := service.Clean() | ||
| if err != nil { | ||
| t.Fatalf("service.Clean %q: %v", service.Name, err) | ||
| } | ||
| }(service) | ||
|
|
||
| resp, err := service.Request("GET", "/are_you_ready") | ||
| if err != nil { | ||
| t.Fatalf("request: %v", err) | ||
| } | ||
|
|
||
| out, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| t.Fatalf("io.ReadAll: %v", err) | ||
| } | ||
|
|
||
| if got, want := string(out), "HEALTHY"; got != want { | ||
| t.Errorf("body: got %q, want %q", got, want) | ||
| } | ||
|
|
||
| if got := resp.StatusCode; got != http.StatusOK { | ||
| t.Errorf("response status: got %d, want %d", got, http.StatusOK) | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
| c, err := storage.NewClient(ctx) | ||
| if err != nil { | ||
| t.Fatalf("storage.NewClient: %v", err) | ||
| } | ||
| defer c.Close() | ||
| bucketName := os.Getenv("GOLANG_SAMPLES_PROJECT_ID") + "-" + service.Version() | ||
| t.Logf("Deleting bucket: %s", bucketName) | ||
|
|
||
| err = testutil.DeleteBucketIfExists(ctx, c, bucketName) | ||
| if err != nil { | ||
| t.Fatalf("testutil.DeleteBucketIfExists: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between TestServiceHealthHttp and this new TestServiceHealthGrpc test. Consider refactoring them into a single table-driven test to improve maintainability and reduce boilerplate.
You could define a slice of test cases, with each case specifying the readiness probe configuration, and then loop through them in a single test function using t.Run for each subtest.
Example structure:
func TestServiceHealth(t *testing.T) {
tc := testutil.EndToEndTest(t)
tests := []struct {
name string
probe *cloudrunci.ReadinessProbe
}{
{
name: "HTTP",
probe: &cloudrunci.ReadinessProbe{
// ... HTTP probe config
},
},
{
name: "gRPC",
probe: &cloudrunci.ReadinessProbe{
// ... gRPC probe config
},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
t.Parallel()
// ... shared test logic using test.probe
})
}
}…o have grpc support
neilalberg
force-pushed
the
grpc_readiness
branch
from
d008f50 to
7cae04c
Compare
1 participant
Description
Update Cloud Run sample service with readiness probes to support GRPC
Checklist
go test -v ./..(see Testing)run/testingdirectory:GOLANG_SAMPLES_E2E_TEST=true GOLANG_SAMPLES_PROJECT_ID=lihenan-s8s-networking-test go test -run ^TestServiceHealthGrpc$ -v .gofmt(see Formatting)go vet(see Formatting)