-
Notifications
You must be signed in to change notification settings - Fork 15
tvc: init CLI #74
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.
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
base: main
Are you sure you want to change the base?
tvc: init CLI #74
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
r-n-o
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at the details of each command, just browsed through top-to-bottom to get a lay of the land. First impression: looks great! 👏
| let is_internal = http_opts | ||
| .contains("option (google.api.method_visibility).restriction = \"INTERNAL\""); | ||
| let is_tvc = fn_name.contains("tvc"); | ||
| if is_internal && !is_tvc { | ||
| // Skip internal-only endpoints (except TVC endpoints) | ||
| continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could make these endpoint non-internal, they're feature-flagged already. So there's no real "danger". I'm just a bit cautious because of the other SDKs (JS, Golang, etc...users might get confused if they see "TVC" activities. With the right docstring though ("BETA"), maybe not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favor of keeping them internal and having some extra complexity here. I don't want them leaking into API spec docs and then confusing people
| @@ -0,0 +1,41 @@ | |||
| # Experimental CLI for Turnkey Verifiable Cloud | |||
|
|
|||
| ## Usage | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I'm wondering: possible to have e2e tests for tvc CLI? We can start small, but I think having a harness will pay off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see them! They're here, but at the very end of the diff (tvc/tests)
🐐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are pretty simple for now and don't fully test functionality with the API - but they should at least check basics like the expected arguments
| /// Run the deploy status command. | ||
| pub async fn run(args: Args) -> anyhow::Result<()> { | ||
| println!("Getting status for deploy: {}", args.deploy_id); | ||
| todo!("deploy status not yet implemented") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be possible with the latest protos? There's a GetDeployment endpoint you can leverage. Or maybe you mean kubernetes level status?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I was originally thinking this would be for k8s type status. We implicitly call GetDeployment in in deploy approve
Maybe what I can do is fill this current stub with GetDeployment, and then we can add another API call and combine the info
tvc/src/config/turnkey.rs
Outdated
| /// API key stored in api_key.json | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct ApiKey { | ||
| /// Hex-encoded compressed public key | ||
| pub public_key: String, | ||
| /// Hex-encoded private key | ||
| pub private_key: String, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth reusing the ApiKey struct from here?
rust-sdk/api_key_stamper/src/lib.rs
Lines 56 to 60 in 06a000d
| /// Represents a Turnkey API key using the P-256 curve. | |
| #[derive(Debug, PartialEq)] | |
| pub struct TurnkeyP256ApiKey { | |
| signing_key: P256SigningKey, | |
| } |
One thing you're missing if you want this to be generic is the curve / scheme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I will add curve so we are more generic
- The idea with this struct is that is represents how its stored on disk. While the TurnkeyP256ApiKey is the abstraction for actually using/generating the p256 key. I'll rename it to StoredApiKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tvc/src/config/turnkey.rs
Outdated
| /// Operator key stored in operator.json | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct OperatorKey { | ||
| /// Hex-encoded compressed public key | ||
| pub public_key: String, | ||
| /// Hex-encoded private key | ||
| pub private_key: String, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here I think a scheme could be beneficial. Although...if you make the struct "QOSOperatorKey" it's clear enough that it'll be whatever QOS expects, which is a seed + 2 derived keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tvc/src/config/turnkey.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl ApiKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These impl should probably move to their own module so we can test them in isolation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Something that can do key pair operations with the QOS p256 scheme. | ||
| pub trait Pair: Send + Sync { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a trait as opposed to implementing on QOSOperatorKey? Do you envision a lot of "something"s? :D
EDIT: oh, I see the vision. Of course: remote operator keys (vs local ones)
Disregard my comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yubikey, TKMS, Password encrypted file etc
Introduce TVC CLI with login, app, and deployment commands
Supersedes
app {init, create},deployment {init, create}, and updatedeployment approve#70