Re: [pbs-devel] [PATCH proxmox-backup] fix #4343: updated `view_task_result` to bail on task failure

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Tue, Aug 22, 2023 at 12:36:03PM +0200, Gabriel Goller wrote:
> Now we make an additional request on `api2/json/.../tasks/{upid}/status` to
> get the `exitstatus` of the task. This allows us to `bail` and thus
> get a non-zero exit code in the cli.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  pbs-client/src/task_log.rs | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/pbs-client/src/task_log.rs b/pbs-client/src/task_log.rs
> index 0bbb928d..4fb31fea 100644
> --- a/pbs-client/src/task_log.rs
> +++ b/pbs-client/src/task_log.rs
> @@ -64,10 +64,24 @@ pub async fn display_task_log(
>              let path = format!("api2/json/nodes/localhost/tasks/{upid_encoded}/log");
>              let result = client.get(&path, Some(param)).await?;
>  
> +            let status_path = format!("api2/json/nodes/localhost/tasks/{upid_encoded}/status");
> +            let status_result = client.get(&status_path, None).await?;
> +

^ shouldn't `active` become false before this can happen?
so can we not just do this once after the loop?

>              let active = result["active"].as_bool().unwrap();
>              let total = result["total"].as_u64().unwrap();
>              let data = result["data"].as_array().unwrap();
>  
> +            if status_result["data"]["status"].as_str() == Some("stopped")
> +                && status_result["data"]["exitstatus"].as_str() != Some("OK")
> +            {
> +                bail!(
> +                    "{}",
> +                    data.iter()
> +                        .map(|d| try_strip_date(d["t"].as_str().unwrap()))
> +                        .fold(String::new(), |a, b| a + " " + b)

^ not a fan of `+` for string concatenation.
`format!()` has at least a chance to figure out the lengths first,
whereas with `+` you're technically doing multiple independent
operations.
Further, you're not adding the newlines in between like it happens from
the use of `print<ln>` in the loop down below ;-)

Also this seems to just use "whatever rest we had not printed yet" as an
_error_ message. If we check the status after the loop, we could just use
a generic bail!("task failed") as the output was already there.

Unless there's a reason to do it this way instead? But I don't think we
can really know how much of it even _is_ error text. We might be getting
up to 500 (`limit` is 500) lines of random text :-)


> +                );
> +            }
> +
>              let lines = data.len();
>  
>              for item in data {
> @@ -76,9 +90,8 @@ pub async fn display_task_log(
>                  if n != start {
>                      bail!("got wrong line number in response data ({n} != {start}");
>                  }
> -                if strip_date && t.len() > 27 && &t[25..27] == ": " {
> -                    let line = &t[27..];
> -                    println!("{line}");
> +                if strip_date {
> +                    println!("{}", try_strip_date(t));
>                  } else {
>                      println!("{t}");
>                  }
> @@ -127,3 +140,11 @@ pub async fn view_task_result(
>  
>      Ok(())
>  }
> +
> +fn try_strip_date(log_msg: &str) -> &str {
> +    if log_msg.len() > 27 && &log_msg[25..27] == ": " {
> +        &log_msg[27..]
> +    } else {
> +        log_msg
> +    }
> +}
> -- 
> 2.39.2