Skip to content

Versioning details in exporter to controller and client #29

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Versioning details in exporter to controller and client #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96cd113
Versioning details in exporter to controller and client
mangelajo Oct 8, 2025
fb533ab
Add GetServiceVersion for both client and exporter
mangelajo Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions proto/jumpstarter/client/v1/client.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ service ClientService {
option (google.api.http) = {delete: "/v1/{name=namespaces/*/leases/*}"};
option (google.api.method_signature) = "name";
}

rpc GetServiceVersion(google.protobuf.Empty) returns (GetServiceVersionResponse) {
option (google.api.http) = {get: "/v1/version"};
option (google.api.method_signature) = "";
}
Comment on lines +59 to +63
Copy link

@coderabbitai coderabbitai bot Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.proto

Length 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.

Suggested change
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.

Copy link
Member Author

@mangelajo mangelajo Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GetServiceVersionResponse from the common module does not have any grpc gateway annotations

Copy link
Member Author

@mangelajo mangelajo Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I have an empty api method signature for grpc gateway?

Copy link

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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.

Copy link

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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

  1. google.protobuf.Empty documentation.
  2. API Linter / AIP-4232 method_signature rules.
  3. gRPC-Gateway annotation guidance (http rule vs. signatures).
  4. 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 Exporter {
Expand Down Expand Up @@ -168,3 +173,9 @@ message DeleteLeaseRequest {
(google.api.resource_reference) = {type: "jumpstarter.dev/Lease"}
];
}

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];
}
Comment on lines +177 to +181
Copy link

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.

8 changes: 7 additions & 1 deletion proto/jumpstarter/v1/common.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,10 @@ enum LogSource {
LOG_SOURCE_BEFORE_LEASE_HOOK = 2; // beforeLease hook execution logs
LOG_SOURCE_AFTER_LEASE_HOOK = 3; // afterLease hook execution logs
LOG_SOURCE_SYSTEM = 4; // System/exporter logs
}
}

message GetServiceVersionResponse {
string version = 1;
string build_date = 2;
string git_commit = 3;
}
7 changes: 7 additions & 0 deletions proto/jumpstarter/v1/jumpstarter.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ import "jumpstarter/v1/common.proto";

// A service where a exporter can connect to make itself available
service ControllerService {
// Get service version1
rpc GetServiceVersion(google.protobuf.Empty) returns (GetServiceVersionResponse);

Comment on lines +16 to +18
Copy link

@coderabbitai coderabbitai bot Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
// 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.

// Exporter registration
rpc Register(RegisterRequest) returns (RegisterResponse);

Expand Down Expand Up @@ -61,6 +64,7 @@ message RegisterRequest {
// jumpstarter.dev/hostname=
// jumpstarter.dev/name=
repeated DriverInstanceReport reports = 2;
string version = 3;
}

message DriverInstanceReport {
Expand All @@ -69,6 +73,8 @@ message DriverInstanceReport {
map<string, string> labels = 3;
optional string description = 4; // optional custom driver description for CLI
map<string, string> methods_description = 5; // method name -> help text for CLI
string version = 6; // driver software version
string minimum_cli_version = 7; // the oldest compatible software version for the client side
}

message RegisterResponse {
Expand Down Expand Up @@ -143,6 +149,7 @@ message GetReportResponse {
// jumpstarter.dev/name=
repeated DriverInstanceReport reports = 3;
repeated Endpoint alternative_endpoints = 4;
string version = 5; // exporter software version
}

message Endpoint {
Expand Down
6 changes: 6 additions & 0 deletions proto/jumpstarter/v1/router.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,19 @@ syntax = "proto3";

package jumpstarter.v1;

import "google/protobuf/empty.proto";

import "jumpstarter/v1/common.proto";

// StreamService
// Claims:
// iss: jumpstarter controller
// aud: jumpstarter router
// sub: jumpstarter client/exporter
// stream: stream id
service RouterService {
// Get service version
rpc GetServiceVersion(google.protobuf.Empty) returns (GetServiceVersionResponse);
// Stream connects caller to another caller of the same stream
rpc Stream(stream StreamRequest) returns (stream StreamResponse);
}
Expand Down