diff --git a/rust/cube-cli/README.md b/rust/cube-cli/README.md index df7843f5577b8..015090e438c39 100644 --- a/rust/cube-cli/README.md +++ b/rust/cube-cli/README.md @@ -28,8 +28,17 @@ pins a specific release tag). Every run checks GitHub for a newer release in the background and prints a notice when one is available (set `CUBE_NO_UPDATE_CHECK=1` to disable, e.g. -in CI; the notice only goes to interactive terminals, on stderr). Update -in place any time with: +in CI; the notice only goes to interactive terminals, on stderr). + +The same check feeds a hint under API errors: when a request fails on the API +side *and* this binary is behind, the error is followed by a line suggesting +`cube update`, since a CLI that lags the API is a common cause of otherwise +puzzling failures. A CLI already on the latest release is never told to +update, and `CUBE_NO_UPDATE_CHECK=1` silences the hint along with the notice. +Unlike the notice, the hint is not limited to interactive terminals — it is +attached to a failure, and a stale pinned CLI in CI is where it pays off. + +Update in place any time with: ```bash cube update # download the latest release and replace this binary diff --git a/rust/cube-cli/src/client.rs b/rust/cube-cli/src/client.rs index f5a8475c47edc..dfba417ec81a1 100644 --- a/rust/cube-cli/src/client.rs +++ b/rust/cube-cli/src/client.rs @@ -4,6 +4,7 @@ use anyhow::{anyhow, bail, Result}; use reqwest::{Method, StatusCode}; use serde_json::Value; +use crate::error::api_bail; use crate::oauth; /// Thin HTTP client over the Cube Cloud public REST API. @@ -196,12 +197,12 @@ impl Client { }) .unwrap_or_else(|| text.clone()); match status { - StatusCode::UNAUTHORIZED => bail!( + StatusCode::UNAUTHORIZED => api_bail!( "unauthorized (401): session expired — run `cube login` (or set CUBE_API_KEY). {detail}" ), - StatusCode::FORBIDDEN => bail!("forbidden (403): {detail}"), - StatusCode::NOT_FOUND => bail!("not found (404): {method} {path}. {detail}"), - _ => bail!("{method} {path} failed with {status}: {detail}"), + StatusCode::FORBIDDEN => api_bail!("forbidden (403): {detail}"), + StatusCode::NOT_FOUND => api_bail!("not found (404): {method} {path}. {detail}"), + _ => api_bail!("{method} {path} failed with {status}: {detail}"), } } @@ -215,7 +216,7 @@ impl Client { let looks_like_html = trimmed.len() >= 2 && trimmed.starts_with('<') && !trimmed.starts_with(") let _ = config.save(); } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::error::is_api_error; + + fn client() -> Client { + Client::new("https://example.cubecloud.dev", "sk-test").unwrap() + } + + fn finish(status: StatusCode, body: &str) -> Result { + client().finish_response( + &Method::GET, + "/api/v1/deployments", + status, + body.to_string(), + ) + } + + #[test] + fn unsuccessful_status_is_an_api_error() { + let err = finish(StatusCode::BAD_REQUEST, r#"{"message":"unknown field"}"#).unwrap_err(); + assert!(is_api_error(&err)); + assert!(err.to_string().contains("unknown field"), "{err}"); + } + + #[test] + fn every_mapped_status_is_an_api_error() { + for status in [ + StatusCode::UNAUTHORIZED, + StatusCode::FORBIDDEN, + StatusCode::NOT_FOUND, + StatusCode::INTERNAL_SERVER_ERROR, + ] { + let err = finish(status, "").unwrap_err(); + assert!(is_api_error(&err), "{status} should be an API error"); + } + } + + #[test] + fn web_app_html_instead_of_json_is_an_api_error() { + let err = finish(StatusCode::OK, "").unwrap_err(); + assert!(is_api_error(&err)); + } + + #[test] + fn successful_responses_still_parse() { + assert_eq!( + finish(StatusCode::OK, r#"{"items":[]}"#).unwrap(), + serde_json::json!({"items": []}) + ); + assert_eq!(finish(StatusCode::NO_CONTENT, "").unwrap(), Value::Null); + } +} diff --git a/rust/cube-cli/src/error.rs b/rust/cube-cli/src/error.rs new file mode 100644 index 0000000000000..35503e864ab6f --- /dev/null +++ b/rust/cube-cli/src/error.rs @@ -0,0 +1,72 @@ +/// An error that came from an API response — an unsuccessful status, or a +/// body the CLI could not make sense of — as opposed to a local failure or a +/// transport error (DNS, TLS, connection refused). +/// +/// It is a distinct error type so `main` can recognize it in the error chain +/// and suggest an update: skew between an older CLI and a newer API is a +/// common cause of otherwise puzzling API errors. +#[derive(Debug)] +pub struct ApiError(String); + +impl std::fmt::Display for ApiError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +impl std::error::Error for ApiError {} + +/// Build an [`ApiError`] as an `anyhow::Error`, ready to return or wrap in +/// further context. +pub fn api_error(message: impl Into) -> anyhow::Error { + anyhow::Error::new(ApiError(message.into())) +} + +/// `bail!` for API failures: returns an [`ApiError`] instead of a plain one. +macro_rules! api_bail { + ($($arg:tt)*) => { + return Err($crate::error::api_error(format!($($arg)*))) + }; +} + +pub(crate) use api_bail; + +/// True when `err`, or anything it wraps, came from an API response. +pub fn is_api_error(err: &anyhow::Error) -> bool { + err.chain().any(|e| e.is::()) +} + +#[cfg(test)] +mod tests { + use super::*; + use anyhow::{anyhow, Context as _}; + + #[test] + fn recognizes_api_errors() { + assert!(is_api_error(&api_error("GET /x failed with 400"))); + } + + #[test] + fn recognizes_api_errors_wrapped_in_context() { + let err = Err::<(), _>(api_error("GET /x failed with 400")) + .context("while listing deployments") + .unwrap_err(); + assert!(is_api_error(&err)); + } + + #[test] + fn ignores_other_errors() { + assert!(!is_api_error(&anyhow!("no config file found"))); + assert!(!is_api_error( + &anyhow!("connection refused").context("request to https://x failed") + )); + } + + #[test] + fn displays_the_message_it_was_built_with() { + assert_eq!( + api_error("GET /x failed with 400").to_string(), + "GET /x failed with 400" + ); + } +} diff --git a/rust/cube-cli/src/main.rs b/rust/cube-cli/src/main.rs index a6bd3181cddc5..e6cf069e8c10f 100644 --- a/rust/cube-cli/src/main.rs +++ b/rust/cube-cli/src/main.rs @@ -1,6 +1,7 @@ mod client; mod commands; mod config; +mod error; mod oauth; mod output; mod telemetry; @@ -281,6 +282,9 @@ async fn main() { let command_name = command.name(); let result = run(global, command).await; + // API failures are commonly caused by a CLI that lags the API, so they + // get an extra "try updating" hint below the error. + let api_error = result.as_ref().err().is_some_and(error::is_api_error); if !is_completion { let mut props = serde_json::Map::new(); @@ -295,11 +299,20 @@ async fn main() { } telemetry::flush().await; } - if let Some(check) = check { - update::print_notice(check).await; - } - if let Err(err) = result { + // Print the failure before consulting the release check: only the hint + // needs that answer, and a slow or unreachable GitHub must not sit between + // the user and the error they are waiting for. + if let Err(err) = &result { eprintln!("error: {err:#}"); + } + let outcome = update::resolve(check, api_error).await; + let announced = update::print_notice(&outcome); + if result.is_err() { + if api_error { + if let Some(hint) = update::api_error_hint(&outcome, announced) { + eprintln!("{hint}"); + } + } std::process::exit(1); } } diff --git a/rust/cube-cli/src/oauth.rs b/rust/cube-cli/src/oauth.rs index 812feafe642c0..27b3dbadb2d30 100644 --- a/rust/cube-cli/src/oauth.rs +++ b/rust/cube-cli/src/oauth.rs @@ -1,8 +1,10 @@ use std::time::{Duration, Instant}; -use anyhow::{anyhow, bail, Result}; +use anyhow::{bail, Result}; use serde::Deserialize; +use crate::error::{api_bail, api_error}; + /// OAuth 2.0 Device Authorization Grant (RFC 8628). /// /// Cube Cloud exposes a confidential authorization-code + device server under @@ -99,13 +101,16 @@ pub async fn request_device_code( let status = res.status(); let text = res.text().await.unwrap_or_default(); if !status.is_success() { - bail!( + api_bail!( "device authorization request failed ({status}) at {endpoint}: {}", text.trim() ); } - serde_json::from_str(&text) - .map_err(|e| anyhow!("could not parse device authorization response: {e}\n{text}")) + serde_json::from_str(&text).map_err(|e| { + api_error(format!( + "could not parse device authorization response: {e}\n{text}" + )) + }) } /// Step 3 — poll the token endpoint until the user approves (or it fails). @@ -144,7 +149,7 @@ pub async fn poll_for_token( if status.is_success() { return serde_json::from_str(&text) - .map_err(|e| anyhow!("could not parse token response: {e}\n{text}")); + .map_err(|e| api_error(format!("could not parse token response: {e}\n{text}"))); } // RFC 8628 §3.5: pending/slow_down keep polling; anything else is fatal. @@ -166,7 +171,7 @@ pub async fn poll_for_token( .unwrap_or_default() ), }, - Err(_) => bail!( + Err(_) => api_bail!( "token poll failed ({status}) at {endpoint}: {}", text.trim() ), @@ -196,10 +201,10 @@ pub async fn refresh( let status = res.status(); let text = res.text().await.unwrap_or_default(); if !status.is_success() { - bail!("token refresh failed ({status}): {}", text.trim()); + api_bail!("token refresh failed ({status}): {}", text.trim()); } serde_json::from_str(&text) - .map_err(|e| anyhow!("could not parse refresh response: {e}\n{text}")) + .map_err(|e| api_error(format!("could not parse refresh response: {e}\n{text}"))) } /// Best-effort attempt to open a URL in the user's browser (no extra deps). diff --git a/rust/cube-cli/src/update.rs b/rust/cube-cli/src/update.rs index bd6bf09679b3c..869f5dd8861e6 100644 --- a/rust/cube-cli/src/update.rs +++ b/rust/cube-cli/src/update.rs @@ -2,7 +2,7 @@ use std::io::IsTerminal as _; use std::time::Duration; use anyhow::{anyhow, bail, Result}; -use owo_colors::OwoColorize; +use owo_colors::{OwoColorize, Style}; use serde::Deserialize; /// GitHub repository that hosts CLI release assets. Overridable for testing. @@ -93,43 +93,138 @@ fn newer_than(candidate: &str, current: &str) -> bool { a > b } -/// Spawn a background check for a newer release. Await the returned handle -/// after the command finishes; it resolves to a printable notice, or `None`. -/// Failures (offline, rate limit) resolve silently to `None`. -pub fn spawn_check() -> tokio::task::JoinHandle> { +/// Outcome of the background release check. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum UpdateCheck { + /// A release newer than this binary is available. + Newer(String), + /// This binary is already on the latest release. + UpToDate, + /// Checks are opted out of via `CUBE_NO_UPDATE_CHECK`. + Disabled, + /// The check could not be completed — offline, rate limited, or slower + /// than the command it ran alongside. + Unknown, +} + +/// Spawn a background check for a newer release. Resolve the returned handle +/// with [`resolve`] once the command finishes. Failures (offline, rate limit) +/// resolve to [`UpdateCheck::Unknown`] rather than surfacing an error. +pub fn spawn_check() -> tokio::task::JoinHandle { tokio::spawn(async { if std::env::var_os("CUBE_NO_UPDATE_CHECK").is_some() { - return None; + return UpdateCheck::Disabled; } - let http = reqwest::Client::builder() + let Ok(http) = reqwest::Client::builder() .user_agent(concat!("cube-cli/", env!("CUBE_CLI_VERSION"))) .build() - .ok()?; - let release = latest_release(&http).await.ok()?; + else { + return UpdateCheck::Unknown; + }; + let Ok(release) = latest_release(&http).await else { + return UpdateCheck::Unknown; + }; let latest = release.version().to_string(); if newer_than(&latest, CURRENT_VERSION) { - Some(format!( - "\n{} {} → {}\nRun {} to install it.", - "A new release of Cube CLI is available:".yellow(), - CURRENT_VERSION.dimmed(), - latest.bold().green(), - "cube update".bold().cyan(), - )) + UpdateCheck::Newer(latest) } else { - None + UpdateCheck::UpToDate } }) } -/// Print a pending update notice (best effort, never blocks long). -pub async fn print_notice(handle: tokio::task::JoinHandle>) { - if !std::io::stderr().is_terminal() { - return; +/// Await the background check, but only when its answer will be used: the +/// notice is interactive-only and the hint only runs on an API failure, so a +/// piped command that succeeded must not pay for a GitHub round trip it will +/// throw away. +pub async fn resolve( + handle: Option>, + api_error: bool, +) -> UpdateCheck { + let Some(handle) = handle else { + return UpdateCheck::Unknown; + }; + if !wanted(api_error, std::io::stderr().is_terminal()) { + return UpdateCheck::Unknown; } - // The check runs concurrently with the command; give a short grace + // The check runs concurrently with the command; give it a short grace // period in case the command finished faster than the API call. - if let Ok(Ok(Some(notice))) = tokio::time::timeout(Duration::from_millis(1500), handle).await { - eprintln!("{notice}"); + match tokio::time::timeout(Duration::from_millis(1500), handle).await { + Ok(Ok(outcome)) => outcome, + _ => UpdateCheck::Unknown, + } +} + +/// Whether anything will read the check's answer — [`print_notice`] on a +/// terminal, or [`api_error_hint`] under an API failure. +fn wanted(api_error: bool, interactive: bool) -> bool { + api_error || interactive +} + +/// Print the "new release available" notice, if there is one to print. +/// Returns whether it was printed. Interactive terminals only — an unprompted +/// nag has no place in piped or logged output. +pub fn print_notice(outcome: &UpdateCheck) -> bool { + if !std::io::stderr().is_terminal() { + return false; + } + let UpdateCheck::Newer(latest) = outcome else { + return false; + }; + eprintln!( + "\n{} {} → {}\nRun {} to install it.", + "A new release of Cube CLI is available:".yellow(), + CURRENT_VERSION.dimmed(), + latest.bold().green(), + "cube update".bold().cyan(), + ); + true +} + +/// Hint printed under an API error. A CLI that lags the API is a common cause +/// of otherwise puzzling API errors, so point at `cube update` — but only when +/// this binary really is behind: telling someone already on the latest release +/// to update is noise that teaches them to ignore the hint. +/// +/// `announced` says whether [`print_notice`] just reported the same release, so +/// the hint can point back at it instead of repeating the version and command. +/// +/// Unlike the notice, this is not limited to interactive terminals: it is +/// attached to a failure the user asked for rather than an unprompted nag, and +/// a stale pinned CLI in CI is exactly where the advice pays off. +pub fn api_error_hint(outcome: &UpdateCheck, announced: bool) -> Option { + hint_for(outcome, announced, std::io::stderr().is_terminal()) +} + +/// Split out from [`api_error_hint`] so the policy can be tested without +/// touching process-wide environment or terminal state. +fn hint_for(outcome: &UpdateCheck, announced: bool, color: bool) -> Option { + let (label, emphasis) = if color { + (Style::new().yellow(), Style::new().bold().cyan()) + } else { + (Style::new(), Style::new()) + }; + let hint = label.style("hint:"); + match outcome { + // Already current, or the user opted out of update checks: nothing to + // suggest that could plausibly resolve the error. + UpdateCheck::UpToDate | UpdateCheck::Disabled => None, + UpdateCheck::Newer(_) if announced => Some(format!( + "{hint} this request failed on the API side — the newer release above may already fix it" + )), + UpdateCheck::Newer(latest) => Some(format!( + "{hint} this request failed on the API side, and Cube CLI {} is available — run {} \ + to upgrade from {CURRENT_VERSION}, then try again", + emphasis.style(latest), + emphasis.style("cube update"), + )), + // Could not find out whether a newer release exists, so the advice is + // worth giving but not worth asserting. + UpdateCheck::Unknown => Some(format!( + "{hint} this request failed on the API side; a newer release may already fix it — run \ + {} to check, then try again", + emphasis.style("cube update"), + )), } } @@ -137,6 +232,55 @@ pub async fn print_notice(handle: tokio::task::JoinHandle>) { mod tests { use super::*; + fn hint(outcome: &UpdateCheck, announced: bool) -> Option { + hint_for(outcome, announced, false) + } + + #[test] + fn a_piped_command_that_succeeded_does_not_wait_on_the_check() { + // Neither consumer would read the answer, so there is nothing to await. + assert!(!wanted(false, false)); + // A terminal gets the notice; an API failure gets the hint. + assert!(wanted(false, true)); + assert!(wanted(true, false)); + } + + #[test] + fn a_current_cli_is_never_told_to_update() { + assert_eq!(hint(&UpdateCheck::UpToDate, false), None); + assert_eq!(hint(&UpdateCheck::Disabled, false), None); + } + + #[test] + fn a_stale_cli_is_pointed_at_the_new_release() { + let hint = hint(&UpdateCheck::Newer("9.9.9".into()), false).unwrap(); + assert!(hint.contains("cube update"), "{hint}"); + assert!(hint.contains("9.9.9"), "{hint}"); + assert!(hint.contains(CURRENT_VERSION), "{hint}"); + } + + #[test] + fn an_announced_release_is_referenced_rather_than_repeated() { + let hint = hint(&UpdateCheck::Newer("9.9.9".into()), true).unwrap(); + assert!(!hint.contains("cube update"), "{hint}"); + assert!(hint.contains("above"), "{hint}"); + } + + #[test] + fn an_undetermined_check_suggests_looking_rather_than_asserting() { + let hint = hint(&UpdateCheck::Unknown, false).unwrap(); + assert!(hint.contains("cube update"), "{hint}"); + assert!(hint.contains("may"), "{hint}"); + } + + #[test] + fn color_is_applied_only_when_asked_for() { + let plain = hint_for(&UpdateCheck::Unknown, false, false).unwrap(); + let colored = hint_for(&UpdateCheck::Unknown, false, true).unwrap(); + assert!(!plain.contains('\u{1b}'), "{plain}"); + assert!(colored.contains('\u{1b}'), "{colored}"); + } + #[test] fn newer_than_compares_numerically() { assert!(newer_than("1.7.10", "1.7.2"));