Versioning details in exporter to controller and client #29
Conversation
Add driver version and cli version requirements on reports Report exporter version to both server and clients
WalkthroughAdds a GetServiceVersion RPC and response message across controller, client, and router services; introduces version and minimum_cli_version fields to registration/report messages; and adds an optional LogSource field to log streaming responses. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router as RouterService
participant Controller as ControllerService
note right of Router #E8F5E9: new RPC forwarded/handled
Client->>Router: GetServiceVersion(Empty) / HTTP GET /v1/version
Router->>Controller: GetServiceVersion(Empty)
Controller-->>Router: GetServiceVersionResponse { version, build_date, git_commit }
Router-->>Client: GetServiceVersionResponse { version, build_date, git_commit }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
LGTM |
|
This should have a method to let the client/exporter request the server version, even if it's just for informative purposes. |
mangelajo
force-pushed
the
proto-component-versioning
branch
from
1ee6745 to
fb533ab
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
proto/jumpstarter/v1/jumpstarter.proto (3)
65-65: Clarify version format (semver) in RegisterRequestDocument expected format (e.g., SemVer) to avoid ambiguity across implementations.
74-76: Use presence for minimum_cli_versionIf you need to distinguish “unset” from “empty,” mark it optional.
Apply this diff:
message DriverInstanceReport { @@ - string minimum_cli_version = 7; // the oldest compatible software version for the client side + optional string minimum_cli_version = 7; // the oldest compatible software version for the client side }
233-237: Deduplicate GetServiceVersionResponse and prefer Timestamp for build_date
- Avoid duplicating this message in multiple packages; reuse a single type to prevent drift.
- Use google.protobuf.Timestamp for build_date for machine-readable time.
Apply this diff here:
message GetServiceVersionResponse { string version = 1; - string build_date = 2; + google.protobuf.Timestamp build_date = 2; string git_commit = 3; }Then, in client proto, reuse this type (see client comment) or move the message to jumpstarter/v1/common.proto and import from both.
proto/jumpstarter/client/v1/client.proto (2)
60-63: Reuse server response type and drop empty method_signatureReturn the shared message to avoid duplication, and remove the empty signature option.
Apply this diff:
- rpc GetServiceVersion(google.protobuf.Empty) returns (GetServiceVersionResponse) { + rpc GetServiceVersion(google.protobuf.Empty) returns (jumpstarter.v1.GetServiceVersionResponse) { option (google.api.http) = {get: "/v1/version"}; - option (google.api.method_signature) = ""; }And add this import (outside this hunk):
import "jumpstarter/v1/jumpstarter.proto";
177-181: Remove duplicate GetServiceVersionResponseThis message duplicates the server type and risks divergence. Prefer importing and reusing jumpstarter.v1.GetServiceVersionResponse. If you must keep a local type, also switch build_date to google.protobuf.Timestamp to match the server.
Apply this diff to remove:
-message GetServiceVersionResponse { - string version = 1 [(google.api.field_behavior) = OUTPUT_ONLY]; - string build_date = 2 [(google.api.field_behavior) = OUTPUT_ONLY]; - string git_commit = 3 [(google.api.field_behavior) = OUTPUT_ONLY]; -}If you decide to keep it instead, at minimum:
- string build_date = 2 [(google.api.field_behavior) = OUTPUT_ONLY]; + google.protobuf.Timestamp build_date = 2 [(google.api.field_behavior) = OUTPUT_ONLY];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proto/jumpstarter/client/v1/client.proto(2 hunks)proto/jumpstarter/v1/jumpstarter.proto(5 hunks)
🔇 Additional comments (3)
proto/jumpstarter/v1/jumpstarter.proto (3)
16-16: LGTM: Added ControllerService.GetServiceVersionRPC shape is fine and backward compatible.
150-151: LGTM: exporter version on GetReportResponseGood addition; backward compatible.
231-232: No functional change hereLooks like formatting/whitespace.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
proto/jumpstarter/v1/jumpstarter.proto (3)
67-68: Document version format/semantics.Briefly state expected format (e.g., SemVer) and whether prereleases/build metadata are allowed.
76-78: Clarify compatibility semantics for driver version fields.
- version: note format (SemVer).
- minimum_cli_version: define how compatibility is evaluated (>= min, SemVer ranges?).
152-153: Add brief docs for exporter version.Note format (SemVer) and whether this mirrors RegisterRequest.version for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
proto/jumpstarter/client/v1/client.proto(2 hunks)proto/jumpstarter/v1/common.proto(1 hunks)proto/jumpstarter/v1/jumpstarter.proto(4 hunks)proto/jumpstarter/v1/router.proto(1 hunks)
🔇 Additional comments (1)
proto/jumpstarter/v1/jumpstarter.proto (1)
188-189: Confirm LogSource type and optional presence handling across SDKs.Ensure LogSource is defined in jumpstarter/v1/common.proto and optional presence is supported in your codegen targets. Adding at field 4 is backward compatible.
|
|
||
| rpc GetServiceVersion(google.protobuf.Empty) returns (GetServiceVersionResponse) { | ||
| option (google.api.http) = {get: "/v1/version"}; | ||
| option (google.api.method_signature) = ""; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Use the shared type from common.proto and drop empty method_signature.
This RPC should return jumpstarter.v1.GetServiceVersionResponse to stay consistent across services. Also, omit the empty method_signature; leave it unset for Empty requests.
- rpc GetServiceVersion(google.protobuf.Empty) returns (GetServiceVersionResponse) {
+ rpc GetServiceVersion(google.protobuf.Empty) returns (jumpstarter.v1.GetServiceVersionResponse) {
option (google.api.http) = {get: "/v1/version"};
- option (google.api.method_signature) = "";
}Additionally, verify there’s no HTTP path collision with other services exposing GET /v1/version if these are co-hosted behind the same gateway.
🏁 Script executed:
#!/bin/bash
# Locate common.proto
fd common.proto
# Search for GetServiceVersionResponse definitions in all .proto files
rg -n "message GetServiceVersionResponse" -g "*.proto"
# Inspect imports and package declarations in client.proto
sed -n '1,50p' proto/jumpstarter/client/v1/client.protoLength of output: 2149
🏁 Script executed:
#!/bin/bash
# Search for any other uses of GET /v1/version to detect path collisions
rg -n 'get: "/v1/version"' -g '*.proto'Length of output: 96
Use shared GetServiceVersionResponse and remove empty method_signature
- rpc GetServiceVersion(google.protobuf.Empty) returns (GetServiceVersionResponse) {
+ rpc GetServiceVersion(google.protobuf.Empty) returns (jumpstarter.v1.GetServiceVersionResponse) {
option (google.api.http) = {get: "/v1/version"};
- option (google.api.method_signature) = "";
}No other GET /v1/version endpoints detected.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rpc GetServiceVersion(google.protobuf.Empty) returns (GetServiceVersionResponse) { | |
| option (google.api.http) = {get: "/v1/version"}; | |
| option (google.api.method_signature) = ""; | |
| } | |
| rpc GetServiceVersion(google.protobuf.Empty) returns (jumpstarter.v1.GetServiceVersionResponse) { | |
| option (google.api.http) = {get: "/v1/version"}; | |
| } |
🤖 Prompt for AI Agents
In proto/jumpstarter/client/v1/client.proto around lines 59-63, the RPC
currently defines an empty option (google.api.method_signature) and should reuse
the shared GetServiceVersionResponse; remove the option
(google.api.method_signature) = "" line and update the RPC to return the shared
GetServiceVersionResponse type (importing the shared proto if required),
ensuring the remaining HTTP option stays intact.
There was a problem hiding this comment.
The GetServiceVersionResponse from the common module does not have any grpc gateway annotations
There was a problem hiding this comment.
Can I have an empty api method signature for grpc gateway?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual content of GetServiceVersionResponse in common.proto
cat proto/jumpstarter/v1/common.proto | grep -A 10 "message GetServiceVersionResponse"
# Check all .proto files that reference GetServiceVersionResponse
rg -n "GetServiceVersionResponse" -g "*.proto" -C 2
Length of output: 2028
@mangelajo You're absolutely right! The shared GetServiceVersionResponse in common.proto is missing the OUTPUT_ONLY field behavior annotations, while your local definition in client.proto has them correctly applied.
The best approach would be to add the annotations to the shared type in common.proto:
message GetServiceVersionResponse {
string version = 1 [(google.api.field_behavior) = OUTPUT_ONLY];
string build_date = 2 [(google.api.field_behavior) = OUTPUT_ONLY];
string git_commit = 3 [(google.api.field_behavior) = OUTPUT_ONLY];
}Then you can use jumpstarter.v1.GetServiceVersionResponse consistently across all services (router, jumpstarter, and client) and remove the duplicate definition from client.proto. This ensures all services benefit from the proper grpc-gateway annotations.
The router.proto and jumpstarter.proto files are already using the shared type, so adding the annotations there would improve their generated HTTP APIs as well.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Google API method_signature annotation with google.protobuf.Empty best practices
💡 Result:
Short best-practices for using google.api.method_signature when google.protobuf.Empty appears
-
If the RPC request type is google.protobuf.Empty:
- Do not add a google.api.method_signature (there are no request fields to list). Client libraries won’t get meaningful overloads from a signature. Cite: google.protobuf.Empty docs. [1]
-
If the RPC returns google.protobuf.Empty (i.e., no response body):
- You may still use method_signature to expose top‑level request fields that are useful for client-library helpers (include the fields in the signature in the same order you expect callers to supply them). Ensure every field marked REQUIRED in the request appears in the signature. Cite: API Linter / AIP-4232. [2]
-
Follow the AIP/linter constraints for signatures:
- Don’t include repeated fields except (optionally) as the final element of a signature. [2]
- Signatures must include all top‑level request fields with the REQUIRED annotation. [2]
-
gRPC<->HTTP (transcoding) considerations:
- Keep signatures consistent with your google.api.http rules; path/query/body mappings must match the top‑level fields you expose. Don’t expect method_signature to change URL mapping — it only helps generated client overloads. Use service config/http rules for alternate mappings. Cite: gRPC‑Gateway / Service config docs. [3][4]
-
Practical tips:
- Prefer explicit request messages over overloading many signatures; signatures are convenience aliases for client libraries, not the canonical API surface.
- Run the Google API linter (aip.dev) on your protos to catch missing REQUIRED fields, repeated-field placement, and other signature rules. [2]
Sources
- google.protobuf.Empty documentation.
- API Linter / AIP-4232 method_signature rules.
- gRPC-Gateway annotation guidance (http rule vs. signatures).
- Google service-config / transcoding notes.
@mangelajo According to Google API best practices, when an RPC takes google.protobuf.Empty as the request type, you should not include a google.api.method_signature annotation at all—not even an empty one. Client libraries won't generate meaningful overloads from it since there are no request fields to expose.
So the correct approach is to simply omit the line:
rpc GetServiceVersion(google.protobuf.Empty) returns (jumpstarter.v1.GetServiceVersionResponse) {
option (google.api.http) = {get: "/v1/version"};
}Regarding your earlier comment about GetServiceVersionResponse not having gRPC Gateway annotations: that's expected and correct. The google.api.http annotations belong on the RPC methods (like GetServiceVersion), not on the message types (like GetServiceVersionResponse). The HTTP endpoint mapping is defined at the service level, and the response message is automatically serialized/deserialized by the gateway.
The response message only needs google.api.field_behavior = OUTPUT_ONLY annotations on its fields (which it already has in common.proto), but it doesn't need HTTP routing annotations.
| message GetServiceVersionResponse { | ||
| string version = 1 [(google.api.field_behavior) = OUTPUT_ONLY]; | ||
| string build_date = 2 [(google.api.field_behavior) = OUTPUT_ONLY]; | ||
| string git_commit = 3 [(google.api.field_behavior) = OUTPUT_ONLY]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate message; rely on common.proto’s GetServiceVersionResponse.
Defining a second GetServiceVersionResponse in package jumpstarter.client.v1 risks divergence from the canonical one in jumpstarter.v1 (common.proto). Delete this block.
-message GetServiceVersionResponse {
- string version = 1 [(google.api.field_behavior) = OUTPUT_ONLY];
- string build_date = 2 [(google.api.field_behavior) = OUTPUT_ONLY];
- string git_commit = 3 [(google.api.field_behavior) = OUTPUT_ONLY];
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| message GetServiceVersionResponse { | |
| string version = 1 [(google.api.field_behavior) = OUTPUT_ONLY]; | |
| string build_date = 2 [(google.api.field_behavior) = OUTPUT_ONLY]; | |
| string git_commit = 3 [(google.api.field_behavior) = OUTPUT_ONLY]; | |
| } |
🤖 Prompt for AI Agents
In proto/jumpstarter/client/v1/client.proto around lines 177-181, there is a
duplicate GetServiceVersionResponse message that should be removed; delete this
message block and ensure the file imports the canonical jumpstarter.v1
common.proto (if not already) and references
jumpstarter.v1.GetServiceVersionResponse instead of the local type so all usages
align with the shared definition.
| // Get service version1 | ||
| rpc GetServiceVersion(google.protobuf.Empty) returns (GetServiceVersionResponse); | ||
|
|
There was a problem hiding this comment.
Fix typo in comment.
“Get service version1” → “Get service version”.
- // Get service version1
+ // Get service version📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get service version1 | |
| rpc GetServiceVersion(google.protobuf.Empty) returns (GetServiceVersionResponse); | |
| // Get service version | |
| rpc GetServiceVersion(google.protobuf.Empty) returns (GetServiceVersionResponse); |
🤖 Prompt for AI Agents
In proto/jumpstarter/v1/jumpstarter.proto around lines 16 to 18, the RPC comment
contains a typo "Get service version1"; update the comment to read "Get service
version" so it accurately describes the GetServiceVersion RPC. Ensure only the
comment text is changed and no RPC signature is modified.
Add driver version and cli version requirements on reports Report exporter version to both server and clients
Summary by CodeRabbit