diff --git a/Cargo.lock b/Cargo.lock index cd8bd32e..0fea61cd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -491,7 +491,7 @@ dependencies = [ [[package]] name = "config" -version = "0.3.49" +version = "0.3.50" dependencies = [ "base64", "chrono", @@ -598,7 +598,7 @@ checksum = "d0a5c400df2834b80a4c3327b3aad3a4c4cd4de0629063962b03235697506a28" [[package]] name = "crypto" -version = "0.3.49" +version = "0.3.50" dependencies = [ "aes-gcm", "base64", @@ -3316,7 +3316,7 @@ dependencies = [ [[package]] name = "testutils" -version = "0.3.49" +version = "0.3.50" dependencies = [ "pem", "rsa", @@ -3586,7 +3586,7 @@ checksum = "5d99f8c9a7727884afe522e9bd5edbfc91a3312b36a77b5fb8926e4c31a41801" [[package]] name = "tower" -version = "0.3.49" +version = "0.3.50" dependencies = [ "config", "pyo3", @@ -3614,7 +3614,7 @@ dependencies = [ [[package]] name = "tower-api" -version = "0.3.49" +version = "0.3.50" dependencies = [ "reqwest", "serde", @@ -3626,7 +3626,7 @@ dependencies = [ [[package]] name = "tower-cmd" -version = "0.3.49" +version = "0.3.50" dependencies = [ "axum", "bytes", @@ -3696,7 +3696,7 @@ checksum = "121c2a6cda46980bb0fcd1647ffaf6cd3fc79a013de288782836f6df9c48780e" [[package]] name = "tower-package" -version = "0.3.49" +version = "0.3.50" dependencies = [ "async-compression", "config", @@ -3714,7 +3714,7 @@ dependencies = [ [[package]] name = "tower-runtime" -version = "0.3.49" +version = "0.3.50" dependencies = [ "async-trait", "chrono", @@ -3737,7 +3737,7 @@ checksum = "8df9b6e13f2d32c91b9bd719c00d1958837bc7dec474d94952798cc8e69eeec3" [[package]] name = "tower-telemetry" -version = "0.3.49" +version = "0.3.50" dependencies = [ "tracing", "tracing-appender", @@ -3746,7 +3746,7 @@ dependencies = [ [[package]] name = "tower-uv" -version = "0.3.49" +version = "0.3.50" dependencies = [ "async-compression", "async_zip", @@ -3764,7 +3764,7 @@ dependencies = [ [[package]] name = "tower-version" -version = "0.3.49" +version = "0.3.50" dependencies = [ "anyhow", "chrono", diff --git a/Cargo.toml b/Cargo.toml index 78784617..011288e7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ resolver = "2" [workspace.package] edition = "2021" -version = "0.3.49" +version = "0.3.50" description = "Tower is the best way to host Python data apps in production" rust-version = "1.81" authors = ["Brad Heller "] diff --git a/crates/config/src/towerfile.rs b/crates/config/src/towerfile.rs index aa385e1a..703c999d 100644 --- a/crates/config/src/towerfile.rs +++ b/crates/config/src/towerfile.rs @@ -12,6 +12,9 @@ pub struct Parameter { #[serde(default)] pub default: String, + + #[serde(default)] + pub hidden: bool, } #[derive(Deserialize, Serialize, Debug)] @@ -122,6 +125,7 @@ impl Towerfile { name, description, default, + hidden: false, }); } } @@ -272,6 +276,26 @@ mod test { assert_eq!(towerfile.parameters.len(), 2); assert_eq!(towerfile.parameters[0].name, "my_first_param"); assert_eq!(towerfile.parameters[1].name, "my_second_param"); + assert!(!towerfile.parameters[0].hidden); + } + + #[test] + fn test_parses_secret_parameters() { + let toml = r#" + [app] + name = "my-app" + script = "./script.py" + source = ["*.py"] + + [[parameters]] + name = "MY_PARAMETER" + hidden = true + "#; + + let towerfile = crate::Towerfile::from_toml(toml).unwrap(); + assert_eq!(towerfile.parameters.len(), 1); + assert_eq!(towerfile.parameters[0].name, "MY_PARAMETER"); + assert!(towerfile.parameters[0].hidden); } #[test] diff --git a/crates/tower-api/src/models/parameter.rs b/crates/tower-api/src/models/parameter.rs index ce85288d..f53c7b45 100644 --- a/crates/tower-api/src/models/parameter.rs +++ b/crates/tower-api/src/models/parameter.rs @@ -23,6 +23,9 @@ pub struct Parameter { #[serde_as(as = "DefaultOnNull")] #[serde(rename = "name")] pub name: String, + #[serde(default)] + #[serde(rename = "hidden")] + pub hidden: bool, } impl Parameter { @@ -31,6 +34,7 @@ impl Parameter { default, description, name, + hidden: false, } } } diff --git a/crates/tower-api/src/models/run_parameter.rs b/crates/tower-api/src/models/run_parameter.rs index 7e9164d4..f45cc23d 100644 --- a/crates/tower-api/src/models/run_parameter.rs +++ b/crates/tower-api/src/models/run_parameter.rs @@ -20,10 +20,17 @@ pub struct RunParameter { #[serde_as(as = "DefaultOnNull")] #[serde(rename = "value")] pub value: String, + #[serde(default)] + #[serde(rename = "hidden")] + pub hidden: bool, } impl RunParameter { pub fn new(name: String, value: String) -> RunParameter { - RunParameter { name, value } + RunParameter { + name, + value, + hidden: false, + } } } diff --git a/crates/tower-cmd/src/api.rs b/crates/tower-cmd/src/api.rs index f3ddc421..9a3ffaf9 100644 --- a/crates/tower-cmd/src/api.rs +++ b/crates/tower-cmd/src/api.rs @@ -840,7 +840,11 @@ pub async fn create_schedule( let run_parameters = parameters.map(|params| { params .into_iter() - .map(|(key, value)| RunParameter { name: key, value }) + .map(|(key, value)| RunParameter { + name: key, + value, + hidden: false, + }) .collect() }); @@ -874,14 +878,21 @@ pub async fn update_schedule( let run_parameters = parameters.map(|params| { params .into_iter() - .map(|(key, value)| RunParameter { name: key, value }) + .map(|(key, value)| RunParameter { + name: key, + value, + hidden: false, + }) .collect() }); let params = tower_api::apis::default_api::UpdateScheduleParams { id_or_name: schedule_id.to_string(), update_schedule_params: tower_api::models::UpdateScheduleParams { - cron: cron.map(|s| s.clone()), + schema: None, + cron: cron.cloned(), + environment: None, + app_version: None, parameters: run_parameters, ..Default::default() }, 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 95d9440d..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, @@ -889,7 +889,22 @@ IMPORTANT REMINDERS: async fn tower_schedules_list(&self) -> Result { match api::list_schedules(&self.config, None, None).await { Ok(response) => { - Self::json_success(serde_json::json!({"schedules": response.schedules})) + let schedules = response + .schedules + .into_iter() + .map(|mut schedule| { + if let Some(parameters) = schedule.parameters.as_mut() { + for parameter in parameters { + if parameter.hidden { + parameter.value = "[hidden]".to_string(); + } + } + } + schedule + }) + .collect::>(); + + Self::json_success(serde_json::json!({"schedules": schedules})) } Err(e) => Self::error_result("Failed to list schedules", e), } @@ -968,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/output.rs b/crates/tower-cmd/src/output.rs index 272a31da..b4b6eb46 100644 --- a/crates/tower-cmd/src/output.rs +++ b/crates/tower-cmd/src/output.rs @@ -46,6 +46,28 @@ impl OutputMode { static OUTPUT_MODE: OnceLock = OnceLock::new(); static CURRENT_SENDER: Mutex>> = Mutex::new(None); +fn write_to_stdout(msg: &str) { + let mut stdout = io::stdout(); + if let Err(err) = stdout.write_all(msg.as_bytes()) { + if err.kind() == io::ErrorKind::BrokenPipe { + std::process::exit(0); + } + panic!("failed writing to stdout: {err}"); + } + stdout.flush().ok(); +} + +fn write_to_stderr(msg: &str) { + let mut stderr = io::stderr(); + if let Err(err) = stderr.write_all(msg.as_bytes()) { + if err.kind() == io::ErrorKind::BrokenPipe { + std::process::exit(0); + } + panic!("failed writing to stderr: {err}"); + } + stderr.flush().ok(); +} + pub fn set_output_mode(mode: OutputMode) { OUTPUT_MODE.set(mode).ok(); if mode.is_mcp() { @@ -210,8 +232,7 @@ pub fn write(msg: &str) { let clean_msg = msg.trim_end().to_string(); send_to_current_sender(clean_msg); } else { - io::stdout().write_all(msg.as_bytes()).unwrap(); - io::stdout().flush().ok(); + write_to_stdout(msg); } } @@ -433,7 +454,12 @@ pub fn table(headers: Vec, data: Vec>, json_da .separator(separator) .title(headers.iter().map(|h| h.yellow().to_string())); - print_stdout(table).unwrap(); + if let Err(err) = print_stdout(table) { + if err.kind() == io::ErrorKind::BrokenPipe { + std::process::exit(0); + } + panic!("failed writing table to stdout: {err}"); + } } } @@ -513,8 +539,7 @@ pub fn write_update_available_message(latest: &str, current: &str) { ); // Always write version check messages to stderr to avoid polluting stdout - use std::io::{self, Write}; - io::stderr().write_all(line.as_bytes()).unwrap(); + write_to_stderr(&line); } pub fn write_dev_version_message(current: &str, latest: &str) { @@ -527,8 +552,7 @@ pub fn write_dev_version_message(current: &str, latest: &str) { .dimmed() ); - use std::io::{self, Write}; - io::stderr().write_all(line.as_bytes()).unwrap(); + write_to_stderr(&line); } /// newline just outputs a newline. This is useful when you have a very specific formatting you 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 aec97162..bfdd4d13 100644 --- a/crates/tower-cmd/src/schedules.rs +++ b/crates/tower-cmd/src/schedules.rs @@ -71,13 +71,25 @@ pub fn schedules_cmd() -> Command { ) .subcommand( Command::new("delete") - .allow_external_subcommands(true) - .override_usage("tower schedules delete [OPTIONS] ") + .arg( + Arg::new("schedule_id") + .value_parser(value_parser!(String)) + .index(1) + .required(true) + .help("The schedule ID to delete"), + ) .after_help("Example: tower schedules delete 123") .about("Delete a schedule"), ) .subcommand( Command::new("update") + .arg( + Arg::new("id_or_name") + .value_parser(value_parser!(String)) + .index(1) + .required(true) + .help("ID or name of the schedule to update"), + ) .arg( Arg::new("cron") .short('c') @@ -93,8 +105,6 @@ pub fn schedules_cmd() -> Command { .help("Parameters (key=value) to pass to the app") .action(clap::ArgAction::Append), ) - .allow_external_subcommands(true) - .override_usage("tower schedules update [OPTIONS] ") .after_help("Example: tower schedules update 123 --cron \"*/15 * * * *\"") .about("Update an existing schedule"), ) @@ -164,43 +174,31 @@ pub async fn do_create(config: Config, args: &ArgMatches) { } pub async fn do_update(config: Config, args: &ArgMatches) { - let schedule_id = extract_schedule_id("update", args.subcommand()); + 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 = extract_schedule_id("delete", args.subcommand()); + let schedule_id = args.get_one::("schedule_id").expect("schedule_id is required"); output::with_spinner( "Deleting schedule", - api::delete_schedule(&config, &schedule_id), + api::delete_schedule(&config, schedule_id), ) .await; output::success(&format!("Schedule {} deleted", schedule_id)); } -fn extract_schedule_id(subcmd: &str, cmd: Option<(&str, &ArgMatches)>) -> String { - if let Some((id, _)) = cmd { - return id.to_string(); - } - - let line = format!( - "Schedule ID is required. Example: tower schedules {} ", - subcmd - ); - output::die(&line); -} - /// Parses `--parameter` arguments into a HashMap of key-value pairs. /// Handles format like "--parameter key=value" fn parse_parameters(args: &ArgMatches) -> Option> { @@ -228,8 +226,177 @@ fn parse_parameters(args: &ArgMatches) -> Option> { } } - Some(param_map) + if param_map.is_empty() { + None + } else { + Some(param_map) + } } else { None } } + +#[cfg(test)] +mod tests { + use super::{parse_parameters, schedules_cmd}; + + #[test] + fn update_accepts_positional_schedule_id_and_flags() { + let matches = schedules_cmd() + .try_get_matches_from([ + "schedules", + "update", + "sch_123", + "--cron", + "*/10 * * * *", + "--parameter", + "env=prod", + "-p", + "team=platform", + ]) + .expect("update args should parse"); + + let ("update", update_args) = matches.subcommand().expect("expected update subcommand") + else { + panic!("expected update subcommand"); + }; + + assert_eq!( + update_args + .get_one::("id_or_name") + .map(String::as_str), + Some("sch_123") + ); + assert_eq!( + update_args.get_one::("cron").map(String::as_str), + Some("*/10 * * * *") + ); + + let params: Vec<&str> = update_args + .get_many::("parameters") + .expect("expected parameters") + .map(String::as_str) + .collect(); + assert_eq!(params, vec!["env=prod", "team=platform"]); + } + + #[test] + fn update_accepts_equals_sign_flag_forms() { + let matches = schedules_cmd() + .try_get_matches_from([ + "schedules", + "update", + "sch_456", + "--cron=*/5 * * * *", + "--parameter=region=us-east-1", + ]) + .expect("equals-form args should parse"); + + let ("update", update_args) = matches.subcommand().expect("expected update subcommand") + else { + panic!("expected update subcommand"); + }; + + assert_eq!( + update_args + .get_one::("id_or_name") + .map(String::as_str), + Some("sch_456") + ); + assert_eq!( + update_args.get_one::("cron").map(String::as_str), + Some("*/5 * * * *") + ); + assert_eq!( + update_args + .get_many::("parameters") + .expect("expected parameter") + .next() + .map(String::as_str), + Some("region=us-east-1") + ); + } + + #[test] + fn update_requires_schedule_id() { + let result = + schedules_cmd().try_get_matches_from(["schedules", "update", "--cron", "*/15 * * * *"]); + assert!(result.is_err()); + } + + #[test] + fn delete_requires_schedule_id() { + let result = schedules_cmd().try_get_matches_from(["schedules", "delete"]); + assert!(result.is_err()); + } + + #[test] + fn parse_parameters_valid_pairs() { + let matches = schedules_cmd() + .try_get_matches_from([ + "schedules", + "update", + "sch_789", + "--parameter", + "env=prod", + "-p", + "team=platform", + ]) + .expect("update args should parse"); + + let ("update", update_args) = matches.subcommand().expect("expected update subcommand") + else { + panic!("expected update subcommand"); + }; + + let params = parse_parameters(update_args).expect("expected parsed parameters"); + assert_eq!(params.get("env"), Some(&"prod".to_string())); + assert_eq!(params.get("team"), Some(&"platform".to_string())); + } + + #[test] + fn parse_parameters_invalid_entries_return_none() { + let matches = schedules_cmd() + .try_get_matches_from([ + "schedules", + "update", + "sch_789", + "--parameter", + "invalid", + "-p", + "=missing-key", + ]) + .expect("update args should parse"); + + let ("update", update_args) = matches.subcommand().expect("expected update subcommand") + else { + panic!("expected update subcommand"); + }; + + assert_eq!(parse_parameters(update_args), None); + } + + #[test] + fn parse_parameters_mixed_valid_and_invalid_keeps_valid() { + let matches = schedules_cmd() + .try_get_matches_from([ + "schedules", + "update", + "sch_789", + "--parameter", + "env=prod", + "-p", + "invalid", + ]) + .expect("update args should parse"); + + let ("update", update_args) = matches.subcommand().expect("expected update subcommand") + else { + panic!("expected update subcommand"); + }; + + let params = parse_parameters(update_args).expect("expected parsed parameters"); + assert_eq!(params.get("env"), Some(&"prod".to_string())); + assert_eq!(params.len(), 1); + } +} 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/crates/tower/src/lib.rs b/crates/tower/src/lib.rs index 751f4ba6..1549e234 100644 --- a/crates/tower/src/lib.rs +++ b/crates/tower/src/lib.rs @@ -23,8 +23,8 @@ mod bindings { let spec = PackageSpec::from_towerfile(&towerfile); - let runtime = tokio::runtime::Runtime::new() - .map_err(|e| PyRuntimeError::new_err(e.to_string()))?; + let runtime = + tokio::runtime::Runtime::new().map_err(|e| PyRuntimeError::new_err(e.to_string()))?; let output = PathBuf::from(output); @@ -40,8 +40,7 @@ mod bindings { .as_ref() .ok_or_else(|| PyRuntimeError::new_err("package build produced no output file"))?; - std::fs::copy(src, &output) - .map_err(|e| PyRuntimeError::new_err(e.to_string()))?; + std::fs::copy(src, &output).map_err(|e| PyRuntimeError::new_err(e.to_string()))?; Ok(()) }) @@ -53,8 +52,8 @@ mod bindings { /// args: Command line arguments (typically sys.argv). #[pyfunction] fn _run_cli(args: Vec) -> PyResult<()> { - let runtime = tokio::runtime::Runtime::new() - .map_err(|e| PyRuntimeError::new_err(e.to_string()))?; + let runtime = + tokio::runtime::Runtime::new().map_err(|e| PyRuntimeError::new_err(e.to_string()))?; // App::new_from_args() must run inside block_on because // Session::from_config_dir() requires an active tokio reactor. diff --git a/pyproject.toml b/pyproject.toml index 6d669d5b..b0a8f99d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "maturin" [project] name = "tower" -version = "0.3.49" +version = "0.3.50" description = "Tower CLI and runtime environment for Tower." authors = [{ name = "Tower Computing Inc.", email = "brad@tower.dev" }] readme = "README.md" diff --git a/src/tower/cli.py b/src/tower/cli.py index d846ac4a..30272891 100644 --- a/src/tower/cli.py +++ b/src/tower/cli.py @@ -1,7 +1,13 @@ +import signal import sys from ._native import _run_cli def main(): + # When invoked via Python, SIGINT defaults to raising KeyboardInterrupt + # in the interpreter instead of terminating the process immediately. + # Restore default SIGINT behavior so Ctrl+C reliably cancels interactive + # CLI flows (matching behavior of the standalone Rust binary). + signal.signal(signal.SIGINT, signal.SIG_DFL) _run_cli(sys.argv) 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""" diff --git a/tests/tower/test_cli.py b/tests/tower/test_cli.py new file mode 100644 index 00000000..948d4d97 --- /dev/null +++ b/tests/tower/test_cli.py @@ -0,0 +1,24 @@ +import signal + +import tower.cli + + +def test_main_restores_default_sigint_and_calls_native(monkeypatch): + captured = {} + + def fake_signal(sig, handler): + captured["sig"] = sig + captured["handler"] = handler + + def fake_run_cli(argv): + captured["argv"] = argv + + monkeypatch.setattr(tower.cli.signal, "signal", fake_signal) + monkeypatch.setattr(tower.cli, "_run_cli", fake_run_cli) + + tower.cli.main() + + assert captured["sig"] == signal.SIGINT + assert captured["handler"] == signal.SIG_DFL + assert isinstance(captured["argv"], list) + assert len(captured["argv"]) > 0 diff --git a/uv.lock b/uv.lock index 04cbf795..59cb1d32 100644 --- a/uv.lock +++ b/uv.lock @@ -2488,7 +2488,7 @@ wheels = [ [[package]] name = "tower" -version = "0.3.49" +version = "0.3.50" source = { editable = "." } dependencies = [ { name = "attrs" },