-
Notifications
You must be signed in to change notification settings - Fork 114
feat: add non-interactive mode and route management options to CLI #2532
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
feat: add non-interactive mode and route management options to CLI #2532
Conversation
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
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.
PR Summary
This PR adds non-interactive mode and route management options to the Rivet CLI, enabling automated deployments and fine-grained control over route handling.
- Added
skip_route_creation
andkeep_existing_routes
options indeploy.rs
to control route behavior, but these are not properly exposed inactor/create.rs
- New
function
module withlist
andendpoint
commands for route management, butendpoint.rs
has potential issues with route ID vs function name matching - Modified
setup_function_routes
indeploy.rs
to support non-interactive mode with detailed logging, but could benefit from more robust error handling - Added environment credential persistence in
toolchain_ctx.rs
when loading from env vars - Inconsistent naming between 'function' and 'route' throughout the codebase (e.g., command aliases) could cause confusion
8 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
#[clap(long)] | ||
skip_route_creation: Option<bool>, |
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.
style: Missing help text for skip_route_creation option. Add doc comment explaining what this flag does.
#[clap(long)] | ||
keep_existing_routes: Option<bool>, |
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.
style: Missing help text for keep_existing_routes option. Add doc comment explaining what this flag does.
/// List all functions for an environment | ||
#[derive(Parser)] | ||
pub struct Opts { | ||
/// Specify the environment to list function for (will prompt if not specified) |
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.
syntax: Typo in help text: 'list function for' should be 'list functions for'
/// Specify the environment to list function for (will prompt if not specified) | |
/// Specify the environment to list functions for (will prompt if not specified) |
skip_route_creation: None, | ||
keep_existing_routes: None, | ||
non_interactive: false, |
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.
logic: These new deploy options are hardcoded but not exposed in the actor creation CLI. Should add corresponding CLI options if these parameters are meant to be configurable.
async fn get_route( | ||
ctx: &ToolchainCtx, | ||
env: &str, | ||
route_id: &str, |
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.
style: Parameter name route_id
is misleading since it's actually receiving a function name
route_id: &str, | |
function_name: &str, |
let matching_route = routes_response | ||
.routes | ||
.iter() | ||
.find(|route| route.id == *route_id) |
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.
logic: Route lookup compares route.id
with route_id
but receives a function name - these may not match. Should likely compare against a different route field
pub mod endpoint; | ||
pub mod list; | ||
|
||
/// Commands for managing routes |
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.
style: Module comment describes 'routes' but module name is 'function'. Consider updating comment or module name for consistency.
/// Commands for managing routes | ||
#[clap(alias = "f", alias = "func", alias = "route")] | ||
Function { | ||
#[clap(subcommand)] | ||
subcommand: function::SubCommand, | ||
}, |
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.
style: The docstring says 'Commands for managing routes' but the command is named 'Function'. Consider renaming to 'Route' for consistency with the documentation, or update the docstring to match the command name.
/// Get information about a specific route endpoint | ||
Endpoint(function::endpoint::Opts), |
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.
logic: The Endpoint command appears to duplicate functionality that should be under the Function subcommand. Consider moving this under Function instead of having it as a top-level command.
pub skip_route_creation: Option<bool>, | ||
pub keep_existing_routes: Option<bool>, | ||
pub non_interactive: bool, |
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.
style: Consider making skip_route_creation and keep_existing_routes required bools when non_interactive is true
66e1fea
to
9a6121a
Compare
Deploying rivet-hub with
|
Latest commit: |
3cd15b1
|
Status: | ✅ Deploy successful! |
Preview URL: | https://5cb2ba00.rivet-hub-7jb.pages.dev |
Branch Preview URL: | https://feat-add-non-interactive-mod.rivet-hub-7jb.pages.dev |
Deploying rivet with
|
Latest commit: |
98c7762
|
Status: | ✅ Deploy successful! |
Preview URL: | https://c834f55d.rivet.pages.dev |
Branch Preview URL: | https://feat-add-non-interactive-mod.rivet.pages.dev |
9a6121a
to
8dd2e07
Compare
8dd2e07
to
599e320
Compare
599e320
to
6f9b0ac
Compare
6f9b0ac
to
e336e8e
Compare
e336e8e
to
571ab68
Compare
259d912
to
540f9d2
Compare
540f9d2
to
3cd15b1
Compare
3cd15b1
to
259d912
Compare
259d912
to
3cd15b1
Compare
3cd15b1
to
98c7762
Compare
Merge activity
|
…2532) <!-- Please make sure there is an issue that this PR is correlated to. --> ## Changes <!-- If there are frontend changes, please include screenshots. -->
Changes