Support creating required KV stores during application deploys #170
Conversation
There was a problem hiding this comment.
This is a tricky one! I did feel a bit "you are lost in a maze of twisty things called resource, all alike" at times, but I don't have a better name (and if I did it would be outside the scope of this PR), and streamlining the "similar but not quite the same enough" bits that make it challenging to follow would take a significant refactor, which I don't think should hold up this PR. Anyway, I left some suggestions but I'm happy to defer most of the style ones to a later refactor - there is one on line 1058 (and surrounding areas) which I care about, and a couple of things I think might be leftovers, but the rest are nits / think-out-louds.
So, sorry for maundering at you, and LGTM!
src/commands/deploy/database.rs
Outdated
| } | ||
| ResourceType::KeyValueStore => { | ||
| client | ||
| .create_key_value_store(&r, Some(resource_label)) |
There was a problem hiding this comment.
I'm curious why this takes &r and the SQLite arms takes r.clone(). Is the API inconsistent or should it have been &db in the previous code?
There was a problem hiding this comment.
the create APIs for KV and SQL are inconsistent. The SQL one sends db name in the body of the request while the KV one is updated to send the name in the path. I am hoping down the line to update the sql one to be at parity with KV
| .await | ||
| .with_context(|| { | ||
| format!( | ||
| r#"Could not link {resource_type} "{}" to app "{}""#, |
There was a problem hiding this comment.
Oh man... the nesting is getting a bit much for me - I find it hard to keep my mental stack when it gets this deep. Maybe that's a matter for a later refactor though.
src/commands/deploy.rs
Outdated
| .is_none() | ||
| { | ||
| // User canceled terminal interaction | ||
| // TODO: Clean up created databases and kv stores |
There was a problem hiding this comment.
Is this something we need to tackle before we merge this PR?
There was a problem hiding this comment.
I don't think so because we also do not cleanup for SQL and users have the ability to use the CLI to clean up resources with other CLI commands. I'd like to expose get_limit endpoints in the cloud API to support knowing a users limit before we start this flow in case they hit their max on a resource.
| @@ -1,80 +1,110 @@ | |||
| // TODO(kate): rename this module to something more generic like `resource` | |||
There was a problem hiding this comment.
Is there a reason to not do this now? Renaming modules tends to be a pretty straight forward refactor.
There was a problem hiding this comment.
I call this out in the PR description. I didn't want to rename it and make it hard to discern the changes.
Signed-off-by: Kate Goldenring <[email protected]>
Signed-off-by: Kate Goldenring <[email protected]>
kate-goldenring
force-pushed
the
create-kv-stores-during-deploys
branch
from
869f3ed to
9a4981a
Compare
Updates the
deployflow to prompt and create KV stores for an application. The flow matches that of SQL, and I updated the code to share most of the code across SQL and KV using the term inside thedatabase.rsmodule. This file should be renamed toresource.rsin a subsequent PR. I didn't want to rename it and make it hard to discern the changes.Note that the
deploy --key-valuesflag only applies to the default store. Should we introduce a way to set pairs during deploy or deprecate this flag and instead instruct them to use thespin cloud kv set(#171) command to set pairs?TODOs for later PRs:
database.rstoresource.rs