Add sandbox metadata req #150
Conversation
📝 WalkthroughWalkthroughAdds a new GetSandboxMetadata gRPC RPC and messages to GravityControl, adds cert fields to deployment messages, and implements GravityClient.GetSandboxMetadata which builds a request, attaches Bearer auth metadata, and calls the new RPC. Changes
Sequence Diagram(s)(omitted — changes do not introduce a multi-component sequential flow requiring visualization) Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @gravity/proto/gravity.proto:
- Around line 567-568: Fix the truncated comment on the cert field in the
ExistingDeployment message by completing the comment to say "(cert, ca, key)" to
match SandboxMetadataResponse.cert; also review and align the comment on
ExistingDeployment.cert (and any other cert fields) so all cert bundle comments
consistently reflect whether they include key or not (e.g., change the
ExistingDeployment.cert comment from "(cert, ca)" to "(cert, ca, key)" if the
bundle includes the key).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
gravity/proto/gravity.pb.gois excluded by!**/*.pb.gogravity/proto/gravity_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
gravity/grpc_client.gogravity/proto/gravity.proto
🧰 Additional context used
🧬 Code graph analysis (1)
gravity/grpc_client.go (1)
gravity/proto/gravity.pb.go (6)
SandboxMetadataResponse(4187-4195)SandboxMetadataResponse(4208-4208)SandboxMetadataResponse(4223-4225)SandboxMetadataRequest(4126-4133)SandboxMetadataRequest(4146-4146)SandboxMetadataRequest(4161-4163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
gravity/grpc_client.go (1)
2337-2355: LGTM! Implementation follows established patterns.The new
GetSandboxMetadatamethod correctly mirrors the existingGetDeploymentMetadataimplementation pattern:
- Validates control client availability before use
- Properly constructs the request with all required fields
- Attaches authorization metadata via Bearer token
- Delegates context/timeout management to the caller
gravity/proto/gravity.proto (3)
15-17: LGTM! RPC definition is well-structured.The new
GetSandboxMetadataRPC follows the established pattern of the existingGetDeploymentMetadataRPC with appropriate request/response message types.
231-231: LGTM! Field addition maintains wire compatibility.Field number 12 is appropriate and doesn't conflict with existing fields. The
certfield extendsExistingDeploymentto support certificate propagation during state synchronization.
576-589: LGTM! Message definitions are well-structured.The
SandboxMetadataRequestandSandboxMetadataResponsemessages are properly defined with appropriate field types and numbers. The response structure follows the same pattern asDeploymentMetadataResponsewith success/error fields and domain-specific data.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
gravity/proto/gravity.proto (1)
16-17: Consider improving the RPC comment for clarity.The comment
// get sandbox metadata (ie a cert)is informal and may undersell what this RPC provides. Based on the response message, it returnsextra_hostsin addition to the certificate. Consider a more descriptive comment like:- // get sandbox metadata (ie a cert) + // get sandbox metadata for provisioning (extra hosts, optional certificate) rpc GetSandboxMetadata(SandboxMetadataRequest) returns (SandboxMetadataResponse);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gravity/proto/gravity.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (1)
gravity/proto/gravity.proto
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
gravity/proto/gravity.proto (3)
231-231: Clarify the distinction betweendeployment_certandcertfields.
ExistingDeploymentnow has two certificate-related fields:
deployment_cert(field 7): Containscert,key,ca, anddnsnamecert(field 12): PEM certificate bundle (cert only, per comment)Consider adding a comment explaining when each should be used, or whether one is intended to replace the other over time. This will help maintainers understand the intended usage pattern.
576-582: LGTM!The
SandboxMetadataRequestmessage is well-structured with appropriate field types and numbering. Thegenerate_certificateboolean flag provides good flexibility for callers to opt-in to certificate generation.
584-590: LGTM!The
SandboxMetadataResponsefollows the established success/error pattern used by other response messages in this proto file. The structure appropriately returns sandbox-specific metadata without unnecessary fields.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @gravity/proto/gravity.proto:
- Line 231: The comment for the protobuf field cert is misleading: replace "PEM
certificate bundle (just cert, ca + key not required)" with a clearer
description such as "PEM certificate (leaf certificate only; CA and private key
not required)" so the meaning of the string cert = 12; field is unambiguous;
also scan for other occurrences of "bundle" in related fields and make analogous
clarifying edits to keep comments consistent.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gravity/proto/gravity.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (1)
gravity/proto/gravity.proto
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
gravity/proto/gravity.proto (3)
15-17: LGTM!The new
GetSandboxMetadataRPC follows the existing patterns in the service and properly references the request/response message types defined below.
567-567: LGTM!The
cert_bundlefield follows proper proto3 field numbering and the comment clearly documents the expected contents (cert, ca, key).
576-590: LGTM!The new
SandboxMetadataRequestandSandboxMetadataResponsemessages are well-structured and follow the established patterns in this proto file (e.g.,DeploymentMetadataRequest/Response). Field numbering is sequential and the success/error pattern is consistent with other response types.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.