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"""