From c0f642bde894167784c114f4b5bf6e7054693027 Mon Sep 17 00:00:00 2001 From: Ben Lovell Date: Thu, 26 Feb 2026 18:58:40 +0100 Subject: [PATCH] feat: return to first class clap handling Specifically to improve error messages for run params, and to give guidance for what to do when something goes wrong. Additionally, moving back to clap means we get stuff like still working for free (this was confusing claude, and also users). --- crates/tower-cmd/src/apps.rs | 112 +++++++++-------- crates/tower-cmd/src/lib.rs | 2 +- crates/tower-cmd/src/mcp.rs | 4 +- crates/tower-cmd/src/run.rs | 116 ++++++++++++++---- crates/tower-cmd/src/schedules.rs | 28 ++--- crates/tower-cmd/src/secrets.rs | 46 +++---- crates/tower-cmd/src/teams.rs | 27 ++-- tests/integration/features/cli_runs.feature | 4 +- tests/integration/features/steps/cli_steps.py | 15 +++ 9 files changed, 227 insertions(+), 127 deletions(-) diff --git a/crates/tower-cmd/src/apps.rs b/crates/tower-cmd/src/apps.rs index 31652eea..319ded8a 100644 --- a/crates/tower-cmd/src/apps.rs +++ b/crates/tower-cmd/src/apps.rs @@ -14,13 +14,29 @@ pub fn apps_cmd() -> Command { .subcommand(Command::new("list").about("List all of your apps")) .subcommand( Command::new("show") - .allow_external_subcommands(true) - .override_usage("tower apps show [OPTIONS] ") - .after_help("Example: tower apps show hello-world") + .arg( + Arg::new("app_name") + .value_parser(value_parser!(String)) + .index(1) + .required(true) + .help("Name of the app"), + ) .about("Show the details about an app in Tower"), ) .subcommand( Command::new("logs") + .arg( + Arg::new("app_name") + .value_parser(value_parser!(String)) + .index(1) + .required(true) + .help("app_name#run_number"), + ) + .arg( + Arg::new("run_number") + .value_parser(value_parser!(i64)) + .index(2), + ) .arg( Arg::new("follow") .short('f') @@ -28,9 +44,6 @@ pub fn apps_cmd() -> Command { .help("Follow logs in real time") .action(clap::ArgAction::SetTrue), ) - .allow_external_subcommands(true) - .override_usage("tower apps logs [OPTIONS] #") - .after_help("Example: tower apps logs hello-world#11") .about("Get the logs from a previous Tower app run"), ) .subcommand( @@ -54,15 +67,29 @@ pub fn apps_cmd() -> Command { ) .subcommand( Command::new("delete") - .allow_external_subcommands(true) - .override_usage("tower apps delete [OPTIONS] ") - .after_help("Example: tower apps delete hello-world") + .arg( + Arg::new("app_name") + .value_parser(value_parser!(String)) + .index(1) + .required(true) + .help("Name of the app"), + ) .about("Delete an app in Tower"), ) } pub async fn do_logs(config: Config, cmd: &ArgMatches) { - let (name, seq) = extract_app_name_and_run("logs", cmd.subcommand()); + let app_name_raw = cmd.get_one::("app_name").expect("app_name is required"); + let (name, seq) = if let Some((name, num_str)) = app_name_raw.split_once('#') { + let num = num_str.parse::().unwrap_or_else(|_| output::die("Run number must be a number")); + (name.to_string(), num) + } else { + let num = match cmd.get_one::("run_number").copied() { + Some(n) => n, + None => latest_run_number(&config, app_name_raw).await, + }; + (app_name_raw.clone(), num) + }; let follow = cmd.get_one::("follow").copied().unwrap_or(false); if follow { @@ -78,7 +105,7 @@ pub async fn do_logs(config: Config, cmd: &ArgMatches) { } pub async fn do_show(config: Config, cmd: &ArgMatches) { - let name = extract_app_name("show", cmd.subcommand()); + let name = cmd.get_one::("app_name").expect("app_name is required"); match api::describe_app(&config, &name).await { Ok(app_response) => { @@ -191,46 +218,20 @@ pub async fn do_create(config: Config, args: &ArgMatches) { } pub async fn do_delete(config: Config, cmd: &ArgMatches) { - let name = extract_app_name("delete", cmd.subcommand()); + let name = cmd.get_one::("app_name").expect("app_name is required"); - output::with_spinner("Deleting app", api::delete_app(&config, &name)).await; + output::with_spinner("Deleting app", api::delete_app(&config, name)).await; } -/// Extract app name and run number from command -fn extract_app_name_and_run(subcmd: &str, cmd: Option<(&str, &ArgMatches)>) -> (String, i64) { - if let Some((name, _)) = cmd { - if let Some((name, num)) = name.split_once('#') { - return ( - name.to_string(), - num.parse::().unwrap_or_else(|_| { - output::die("Run number must be an actual number"); - }), - ); - } - - let line = format!( - "Run number is required. Example: tower apps {} #", - subcmd - ); - output::die(&line); - } - let line = format!( - "App name is required. Example: tower apps {} #", - subcmd - ); - output::die(&line) -} - -fn extract_app_name(subcmd: &str, cmd: Option<(&str, &ArgMatches)>) -> String { - if let Some((name, _)) = cmd { - return name.to_string(); +async fn latest_run_number(config: &Config, name: &str) -> i64 { + match api::describe_app(config, name).await { + Ok(resp) => resp.runs + .iter() + .map(|r| r.number) + .max() + .unwrap_or_else(|| output::die(&format!("No runs found for app '{}'", name))), + Err(err) => output::tower_error_and_die(err, "Fetching app details failed"), } - - let line = format!( - "App name is required. Example: tower apps {} ", - subcmd - ); - output::die(&line); } const FOLLOW_BACKOFF_INITIAL: Duration = Duration::from_millis(500); @@ -539,9 +540,24 @@ mod tests { assert_eq!(cmd, "logs"); assert_eq!(sub_matches.get_one::("follow"), Some(&true)); assert_eq!( - sub_matches.subcommand().map(|(name, _)| name), + sub_matches.get_one::("app_name").map(|s| s.as_str()), Some("hello-world#11") ); + assert_eq!(sub_matches.get_one::("run_number"), None); + } + + #[test] + fn test_separate_run_number_parsing() { + let matches = apps_cmd() + .try_get_matches_from(["apps", "logs", "hello-world", "11"]) + .unwrap(); + let (_, sub_matches) = matches.subcommand().unwrap(); + + assert_eq!( + sub_matches.get_one::("app_name").map(|s| s.as_str()), + Some("hello-world") + ); + assert_eq!(sub_matches.get_one::("run_number"), Some(&11)); } #[test] diff --git a/crates/tower-cmd/src/lib.rs b/crates/tower-cmd/src/lib.rs index 864fc34f..a1086e28 100644 --- a/crates/tower-cmd/src/lib.rs +++ b/crates/tower-cmd/src/lib.rs @@ -172,7 +172,7 @@ impl App { } Some(("deploy", args)) => deploy::do_deploy(sessionized_config, args).await, Some(("package", args)) => package::do_package(sessionized_config, args).await, - Some(("run", args)) => run::do_run(sessionized_config, args, args.subcommand()).await, + Some(("run", args)) => run::do_run(sessionized_config, args).await, Some(("teams", sub_matches)) => { let teams_command = sub_matches.subcommand(); diff --git a/crates/tower-cmd/src/mcp.rs b/crates/tower-cmd/src/mcp.rs index efe7e2a0..ad6043ae 100644 --- a/crates/tower-cmd/src/mcp.rs +++ b/crates/tower-cmd/src/mcp.rs @@ -651,7 +651,7 @@ impl TowerService { } #[tool( - description = "Run on Tower cloud. Prerequisites: Towerfile, tower_apps_create, tower_deploy." + description = "Run an app on Tower cloud. Pass parameters as a JSON object, e.g. {\"key\": \"value\"}. Prerequisites: Towerfile, tower_apps_create, tower_deploy." )] async fn tower_run_remote( &self, @@ -983,6 +983,8 @@ Rules: - Use tower_file_update/add_parameter to modify Towerfiles (never edit TOML directly) - DO NOT add hatchling/setuptools to pyproject.toml - Tower handles deployment - Tower apps need: pyproject.toml (deps only), Python code, Towerfile +- Pass run parameters as a JSON object {\"key\": \"value\"}, not as CLI flags +- There is no --param or --params flag Use tower_workflow_help for the complete workflow.".to_string()), } diff --git a/crates/tower-cmd/src/run.rs b/crates/tower-cmd/src/run.rs index bec58f5a..b0a79ea7 100644 --- a/crates/tower-cmd/src/run.rs +++ b/crates/tower-cmd/src/run.rs @@ -27,7 +27,19 @@ use tower_runtime::subprocess::SubprocessBackend; pub fn run_cmd() -> Command { Command::new("run") - .allow_external_subcommands(true) + .after_help( + "Examples:\n \ + tower run Run app from ./Towerfile (remote)\n \ + tower run --local Run app from ./Towerfile (local)\n \ + tower run my-app Run a deployed app by name\n \ + tower run -p key=value Pass a parameter to the run\n \ + tower run my-app -p key=value Run a named app with parameters", + ) + .arg( + Arg::new("app_name") + .help("Name of a deployed app to run (uses ./Towerfile if omitted)") + .index(1), + ) .arg( Arg::new("dir") .long("dir") @@ -65,11 +77,26 @@ pub fn run_cmd() -> Command { .about("Run your code in Tower or locally") } -pub async fn do_run(config: Config, args: &ArgMatches, cmd: Option<(&str, &ArgMatches)>) { - if let Err(e) = do_run_inner(config, args, cmd).await { +pub async fn do_run(config: Config, args: &ArgMatches) { + if let Err(e) = do_run_inner(config, args).await { match e { - Error::ApiRunError { source } => { - output::tower_error_and_die(source, "Scheduling run failed"); + Error::ApiRunError { ref source } => { + let is_not_found = matches!( + source, + tower_api::apis::Error::ResponseError(resp) if resp.status == reqwest::StatusCode::NOT_FOUND + ); + if is_not_found { + output::error("App not found. It may not exist or hasn't been deployed yet."); + output::write("\nTo fix this:\n"); + output::write(" 1. Check your app exists: tower apps list\n"); + output::write(" 2. Deploy your app: tower deploy\n"); + output::write(" 3. Then run it: tower run\n"); + std::process::exit(1); + } + if let Error::ApiRunError { source } = e { + output::tower_error_and_die(source, "Scheduling run failed"); + } + unreachable!(); } _ => { output::die(&e.to_string()); @@ -83,9 +110,8 @@ pub async fn do_run(config: Config, args: &ArgMatches, cmd: Option<(&str, &ArgMa pub async fn do_run_inner( config: Config, args: &ArgMatches, - cmd: Option<(&str, &ArgMatches)>, ) -> Result<(), Error> { - let res = get_run_parameters(args, cmd); + let res = get_run_parameters(args); // We always expect there to be an environment due to the fact that there is a // default value. @@ -467,18 +493,15 @@ fn handle_run_completion(res: Result) -> Result< } } -/// get_run_parameters takes care of all the hairy bits around digging about in the `clap` -/// internals to figure out what the user is requesting. In the end, it determines if we are meant -/// to do a local run or a remote run, and it determines the path to the relevant Towerfile that -/// should be loaded. +/// Extracts the local/remote flag, Towerfile directory, parameters, and optional app name +/// from the parsed CLI args. fn get_run_parameters( args: &ArgMatches, - cmd: Option<(&str, &ArgMatches)>, ) -> Result<(bool, PathBuf, HashMap, Option), config::Error> { let local = *args.get_one::("local").unwrap(); let path = resolve_path(args); let params = parse_parameters(args); - let app_name = get_app_name(cmd); + let app_name = args.get_one::("app_name").cloned(); Ok((local, path, params, app_name)) } @@ -528,14 +551,6 @@ fn resolve_path(args: &ArgMatches) -> PathBuf { } } -/// get_app_name is a helper function that will extract the app name from the `clap` arguments if -fn get_app_name(cmd: Option<(&str, &ArgMatches)>) -> Option { - match cmd { - Some((name, _)) if !name.is_empty() => Some(name.to_string()), - _ => None, - } -} - /// get_secrets_inner manages the process of getting secrets from the Tower server in a way that can be /// used by the local runtime during local app execution. Returns API errors for spinner handling. async fn get_secrets( @@ -799,3 +814,62 @@ fn monitor_run_completion(config: &Config, run: &Run) -> oneshot::Receiver rx } + +#[cfg(test)] +mod tests { + use super::run_cmd; + + fn parse(args: &[&str]) -> Result { + let mut full = vec!["run"]; + full.extend_from_slice(args); + run_cmd().try_get_matches_from(full) + } + + #[test] + fn app_name_parsed_as_positional_arg() { + let m = parse(&["my-app"]).unwrap(); + assert_eq!(m.get_one::("app_name").map(|s| s.as_str()), Some("my-app")); + } + + #[test] + fn no_app_name_is_fine() { + let m = parse(&[]).unwrap(); + assert_eq!(m.get_one::("app_name"), None); + } + + #[test] + fn unknown_flags_are_rejected() { + let err = parse(&["--param", "x=y"]).unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("--param"), "should mention the bad flag: {msg}"); + assert!(msg.contains("--parameter"), "should suggest the correct flag: {msg}"); + } + + #[test] + fn help_flag_is_not_swallowed_as_app_name() { + let err = parse(&["--help"]).unwrap_err(); + assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp); + } + + #[test] + fn help_after_app_name_still_shows_help() { + let err = parse(&["my-app", "--help"]).unwrap_err(); + assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp); + } + + #[test] + fn parameters_after_app_name() { + let m = parse(&["my-app", "-p", "key=val"]).unwrap(); + assert_eq!(m.get_one::("app_name").map(|s| s.as_str()), Some("my-app")); + let params: Vec<&String> = m.get_many::("parameters").unwrap().collect(); + assert_eq!(params, vec!["key=val"]); + } + + #[test] + fn parameters_before_app_name() { + let m = parse(&["-p", "key=val", "my-app"]).unwrap(); + assert_eq!(m.get_one::("app_name").map(|s| s.as_str()), Some("my-app")); + let params: Vec<&String> = m.get_many::("parameters").unwrap().collect(); + assert_eq!(params, vec!["key=val"]); + } +} diff --git a/crates/tower-cmd/src/schedules.rs b/crates/tower-cmd/src/schedules.rs index 56a06d70..bfdd4d13 100644 --- a/crates/tower-cmd/src/schedules.rs +++ b/crates/tower-cmd/src/schedules.rs @@ -73,23 +73,22 @@ pub fn schedules_cmd() -> Command { Command::new("delete") .arg( Arg::new("schedule_id") - .required(true) .value_parser(value_parser!(String)) - .help("The schedule ID to delete") - .action(clap::ArgAction::Set), + .index(1) + .required(true) + .help("The schedule ID to delete"), ) - .override_usage("tower schedules delete [OPTIONS] ") .after_help("Example: tower schedules delete 123") .about("Delete a schedule"), ) .subcommand( Command::new("update") .arg( - Arg::new("schedule_id") - .required(true) + Arg::new("id_or_name") .value_parser(value_parser!(String)) - .help("The schedule ID to update") - .action(clap::ArgAction::Set), + .index(1) + .required(true) + .help("ID or name of the schedule to update"), ) .arg( Arg::new("cron") @@ -106,7 +105,6 @@ pub fn schedules_cmd() -> Command { .help("Parameters (key=value) to pass to the app") .action(clap::ArgAction::Append), ) - .override_usage("tower schedules update [OPTIONS] ") .after_help("Example: tower schedules update 123 --cron \"*/15 * * * *\"") .about("Update an existing schedule"), ) @@ -176,21 +174,21 @@ pub async fn do_create(config: Config, args: &ArgMatches) { } pub async fn do_update(config: Config, args: &ArgMatches) { - let schedule_id = args.get_one::("schedule_id").unwrap(); + let id_or_name = args.get_one::("id_or_name").expect("id_or_name is required"); let cron = args.get_one::("cron"); let parameters = parse_parameters(args); output::with_spinner( "Updating schedule", - api::update_schedule(&config, schedule_id, cron, parameters), + api::update_schedule(&config, id_or_name, cron, parameters), ) .await; - output::success(&format!("Schedule {} updated", schedule_id)); + output::success(&format!("Schedule {} updated", id_or_name)); } pub async fn do_delete(config: Config, args: &ArgMatches) { - let schedule_id = args.get_one::("schedule_id").unwrap(); + let schedule_id = args.get_one::("schedule_id").expect("schedule_id is required"); output::with_spinner( "Deleting schedule", @@ -265,7 +263,7 @@ mod tests { assert_eq!( update_args - .get_one::("schedule_id") + .get_one::("id_or_name") .map(String::as_str), Some("sch_123") ); @@ -301,7 +299,7 @@ mod tests { assert_eq!( update_args - .get_one::("schedule_id") + .get_one::("id_or_name") .map(String::as_str), Some("sch_456") ); diff --git a/crates/tower-cmd/src/secrets.rs b/crates/tower-cmd/src/secrets.rs index ecd0b94a..71198856 100644 --- a/crates/tower-cmd/src/secrets.rs +++ b/crates/tower-cmd/src/secrets.rs @@ -73,9 +73,22 @@ pub fn secrets_cmd() -> Command { ) .subcommand( Command::new("delete") - .allow_external_subcommands(true) - .override_usage("tower secrets delete [OPTIONS] ") - .after_help("Example: tower secrets delete MY_API_KEY") + .arg( + Arg::new("secret_name") + .value_parser(value_parser!(String)) + .index(1) + .required(true) + .help("secret name, or environment/secret_name"), + ) + .arg( + Arg::new("environment") + .short('e') + .long("environment") + .default_value("default") + .value_parser(value_parser!(String)) + .help("environment to delete the secret from") + .action(clap::ArgAction::Set), + ) .about("Delete a secret in Tower"), ) } @@ -174,7 +187,13 @@ pub async fn do_create(config: Config, args: &ArgMatches) { } pub async fn do_delete(config: Config, args: &ArgMatches) { - let (environment, name) = extract_secret_environment_and_name("delete", args.subcommand()); + let secret_name_arg = args.get_one::("secret_name").expect("secret_name is required"); + let (environment, name) = if let Some((env, name)) = secret_name_arg.split_once('/') { + (env.to_string(), name.to_string()) + } else { + let env = args.get_one::("environment").expect("environment has default"); + (env.clone(), secret_name_arg.clone()) + }; debug!("deleting secret, environment={} name={}", environment, name); output::with_spinner( @@ -224,22 +243,3 @@ async fn encrypt_and_create_secret( .map_err(SecretCreationError::CreateFailed) } -fn extract_secret_environment_and_name( - subcmd: &str, - cmd: Option<(&str, &ArgMatches)>, -) -> (String, String) { - if let Some((slug, _)) = cmd { - if let Some((env, name)) = slug.split_once('/') { - return (env.to_string(), name.to_string()); - } - - let line = format!( - "Secret name is required. Example: tower secrets {} /", - subcmd - ); - output::die(&line); - } - - let line = format!("Secret name and environment is required. Example: tower secrets {} /", subcmd); - output::die(&line); -} diff --git a/crates/tower-cmd/src/teams.rs b/crates/tower-cmd/src/teams.rs index d631e532..1a5a9174 100644 --- a/crates/tower-cmd/src/teams.rs +++ b/crates/tower-cmd/src/teams.rs @@ -1,4 +1,4 @@ -use clap::{ArgMatches, Command}; +use clap::{value_parser, Arg, ArgMatches, Command}; use colored::*; use config::Config; @@ -11,7 +11,13 @@ pub fn teams_cmd() -> Command { .subcommand(Command::new("list").about("List teams you belong to")) .subcommand( Command::new("switch") - .allow_external_subcommands(true) + .arg( + Arg::new("team_name") + .value_parser(value_parser!(String)) + .index(1) + .required(true) + .help("Name of the team to switch to"), + ) .about("Switch context to a different team"), ) } @@ -82,18 +88,18 @@ pub async fn do_list(config: Config) { } pub async fn do_switch(config: Config, args: &ArgMatches) { - let name = extract_team_name("switch", args.subcommand()); + let name = args.get_one::("team_name").expect("team_name is required"); // Refresh the session first to ensure we have the latest teams data let session = refresh_session(&config).await; // Check if the provided team name exists in the refreshed session - let team = session.teams.iter().find(|team| team.name == name); + let team = session.teams.iter().find(|team| team.name == *name); match team { Some(team) => { // Team found, set it as active - match config.set_active_team_by_name(&name) { + match config.set_active_team_by_name(name) { Ok(_) => { output::success(&format!("Switched to team: {}", team.name)); } @@ -114,14 +120,3 @@ pub async fn do_switch(config: Config, args: &ArgMatches) { } } -fn extract_team_name(subcmd: &str, cmd: Option<(&str, &ArgMatches)>) -> String { - if let Some((name, _)) = cmd { - return name.to_string(); - } - - let line = format!( - "Team name is required. Example: tower teams {} ", - subcmd - ); - output::die(&line); -} diff --git a/tests/integration/features/cli_runs.feature b/tests/integration/features/cli_runs.feature index 4df64d6e..27f272b9 100644 --- a/tests/integration/features/cli_runs.feature +++ b/tests/integration/features/cli_runs.feature @@ -3,10 +3,10 @@ Feature: CLI Run Commands I want to run applications locally and remotely with proper feedback So that I can develop and deploy my applications effectively - Scenario: CLI remote run should show red error message when API fails + Scenario: CLI remote run should show helpful error when app not found Given I have a simple hello world application When I run "tower run" via CLI - Then the final crash status should show red "Error:" + Then the output should show "App not found" with guidance Scenario: CLI local run should have green timestamps and detect crashes Given I have a simple hello world application that exits with code 1 diff --git a/tests/integration/features/steps/cli_steps.py b/tests/integration/features/steps/cli_steps.py index 1d8151f5..e0b71da6 100644 --- a/tests/integration/features/steps/cli_steps.py +++ b/tests/integration/features/steps/cli_steps.py @@ -143,6 +143,21 @@ def step_final_crash_status_should_show_error(context): assert "Error:" in output, f"Expected 'Error:' in crash message, got: {output}" +@step('the output should show "App not found" with guidance') +def step_output_should_show_app_not_found_with_guidance(context): + """Verify 404 error shows helpful guidance instead of a bare error""" + output = context.cli_output + assert ( + red_color_code in output + ), f"Expected red color codes in output, got: {output}" + assert ( + "App not found" in output + ), f"Expected 'App not found' in output, got: {output}" + assert ( + "tower deploy" in output + ), f"Expected 'tower deploy' guidance in output, got: {output}" + + @step('the final status should show "Your local run crashed!" in red') def step_final_status_should_show_crashed_in_red(context): """Verify local run shows 'Your local run crashed!' in red"""