engineering: Harpoon gRPC Protobuf #386
Conversation
frhuelsz
force-pushed
the
user/frhuelsz/grpc/proto
branch
from
2293c03 to
fc7dd46
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Harpoon gRPC protobuf definition file and integrates it with the trident_cli crate. The changes enable gRPC-based communication for Trident servicing operations through a well-structured service interface.
Key Changes
- Added comprehensive gRPC service definition in
proto/harpoon.protowith TridentService covering installation, update, validation, and configuration operations - Integrated protobuf compilation into trident_cli build process with tonic/prost dependencies
- Refactored version handling to support environment-based version override via TRIDENT_VERSION
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/harpoon.proto | New protobuf file defining TridentService with 15 RPC methods, message types, enums for servicing operations |
| crates/trident_cli/build.rs | New build script to compile protobuf definitions using tonic-build |
| crates/trident_cli/src/main.rs | Added protobuf module inclusion and refactored version constant to support environment override |
| crates/trident_cli/src/cli.rs | Updated to use new TRIDENT_VERSION constant |
| crates/trident_cli/Cargo.toml | Added gRPC dependencies (tonic, prost, tokio) and build dependencies; updated version and edition |
| crates/trident/Cargo.toml | Reorganized dependencies, moved optional dependencies to main section |
| Cargo.lock | Updated dependency versions for trident_cli and zstd-related crates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
proto/harpoon.proto
Outdated
|
|
||
| // TridentService provides methods for managing Trident servicing operations | ||
| service TridentService { | ||
| // Install performs a Trident installation operation. |
There was a problem hiding this comment.
Are we phasing out the "clean install" term? I think we tend to use "install"/"installation" now more
There was a problem hiding this comment.
Our documentation uses "clean install" everywhere so I'm in favor of keeping "clean install" in use
There was a problem hiding this comment.
I think install is a more generic name, and "clean install" is a specific subset of installs that was used because we only supported clean installs at the beginning. I'd rather not perpetuate the pattern in new apis, this is also shorter.
proto/harpoon.proto
Outdated
| // filesystem. | ||
| rpc CheckRoot(CheckRootRequest) returns (stream ServicingResponse); | ||
|
|
||
| // Attempts to commit a finalized update. |
There was a problem hiding this comment.
Is there no CommitInstall?
There was a problem hiding this comment.
good q, perhaps there should be just 1 commit? will ask in sync meeting
| FAILURE = 1; | ||
| } | ||
|
|
||
| message FinalStatus { |
There was a problem hiding this comment.
Would this essentially be the Host Status? Or, also some other things?
There was a problem hiding this comment.
basically the final result of the servicing operation, we may edit it to include host status if needed.
| message FinalStatus { | ||
| // Final status of the servicing operation. | ||
| StatusCode status = 1; | ||
| // Message associated with the final status. |
There was a problem hiding this comment.
What is the message in this context? Is that the HS?...
There was a problem hiding this comment.
tbh this we may need to revise as we go
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tonic = "0.14.2" | ||
| tonic-prost = "0.14.2" |
There was a problem hiding this comment.
The trident_cli crate uses tonic version 0.14.2, while the trident crate uses tonic version 0.12.3. This version mismatch could lead to compatibility issues if types from the generated protobuf code need to be shared between these crates or if they interact with common dependencies.
| prost = "0.14.1" | ||
| prost-types = "0.14.1" |
There was a problem hiding this comment.
The trident_cli crate uses prost version 0.14.1, while the trident crate uses prost version 0.13.4. This version mismatch could lead to compatibility issues if protobuf types need to be shared between these crates or if they interact with common dependencies that expect a specific prost version.
| prost = "0.14.1" | |
| prost-types = "0.14.1" | |
| prost = "0.13.4" | |
| prost-types = "0.13.4" |
| None => env!("CARGO_PKG_VERSION"), | ||
| }; | ||
|
|
||
| pub mod protobufs { |
There was a problem hiding this comment.
The protobufs module is declared as public but is not being used anywhere in the visible code. Consider making it private with 'mod protobufs' unless it needs to be exported for use by other modules or crates.
| pub mod protobufs { | |
| mod protobufs { |
🔍 Description
Introduce Harpoon gRPC proto file