Add unit help to all ByteCount fields (#862)

* Add unit help to all ByteCount fields

The help strings for API fields accepting a `ByteCount` have a dilemma.
For users directly consuming the API, or via the SDK, we need to make it
clear that they should use bytes as the type name suggests. For CLI,
however, we want to encourage users to pass unit suffixes for ease of
use. Help text that emphasizes bytes will lead users to write out values
in bytes, an annoying and error-prone process.

With oxidecomputer/omicron#6737 we have updated
the doc strings on all external API `ByteCount` fields to include the
string "(in bytes)". Add a function to update all CLI `ByteCount` args
and remove that string, appending a help message explaining the units we
accept. While we're at it, also harmonize all CLI-specific `ByteCount`
doc strings.

---------

Co-authored-by: Adam Leventhal <[email protected]>

cli/docs/cli.json

@@ -474,7 +474,7 @@
             },
             {
               "long": "size",
-              "help": "The total size of the Disk (in bytes)"
+              "help": "The total size of the Disk; unit suffixes are in powers of two (1k = 1024 bytes) for example: 6GiB, 512k, 2048mib"
             }
           ]
         },
@@ -520,7 +520,7 @@
             },
             {
               "long": "disk-size",
-              "help": "The size of the disk to create. If unspecified, the size of the file will be used, rounded up to the nearest GB"
+              "help": "The size of the disk to create. If unspecified, the size of the file will be used, rounded up to the nearest GB; unit suffixes are in powers of two (1k = 1024 bytes) for example: 6GiB, 512k, 2048mib"
             },
             {
               "long": "image",
@@ -1425,7 +1425,7 @@
             },
             {
               "long": "memory",
-              "help": "The amount of RAM (in bytes) to be allocated to the instance"
+              "help": "The amount of RAM to be allocated to the instance; unit suffixes are in powers of two (1k = 1024 bytes) for example: 6GiB, 512k, 2048mib"
             },
             {
               "long": "name"
@@ -1668,27 +1668,27 @@
           "args": [
             {
               "long": "description",
-              "help": "Description of the instance to create"
+              "help": "Description of the instance"
             },
             {
               "long": "hostname",
-              "help": "Hostname of the instance to create"
+              "help": "The hostname to be assigned to the instance"
             },
             {
               "long": "image",
               "help": "Source image"
             },
             {
               "long": "memory",
-              "help": "Instance memory e.g 32M. Suffix can be k,m,g,t"
+              "help": "Amount of RAM to be allocated to the instance; unit suffixes are in powers of two (1k = 1024 bytes) for example: 6GiB, 512k, 2048mib"
             },
             {
               "long": "name",
               "help": "Name of the instance to create"
             },
             {
               "long": "ncpus",
-              "help": "Instance CPU count"
+              "help": "Number of vCPUs to be allocated to the instance"
             },
             {
               "long": "profile",
@@ -1701,7 +1701,7 @@
             },
             {
               "long": "size",
-              "help": "Boot disk size e.g. 512G. Suffix can be k,m,g,t"
+              "help": "Boot disk size; unit suffixes are in powers of two (1k = 1024 bytes) for example: 6GiB, 512k, 2048mib"
             },
             {
               "long": "start",
@@ -3727,7 +3727,7 @@
                 },
                 {
                   "long": "memory",
-                  "help": "The amount of RAM (in bytes) available for running instances in the Silo"
+                  "help": "The amount of RAM available for running instances in the Silo; unit suffixes are in powers of two (1k = 1024 bytes) for example: 6GiB, 512k, 2048mib"
                 },
                 {
                   "long": "profile",
@@ -3740,7 +3740,7 @@
                 },
                 {
                   "long": "storage",
-                  "help": "The amount of storage (in bytes) available for disks or snapshots"
+                  "help": "The amount of storage available for disks or snapshots; unit suffixes are in powers of two (1k = 1024 bytes) for example: 6GiB, 512k, 2048mib"
                 }
               ]
             },

cli/src/cli_builder.rs

@@ -4,19 +4,19 @@
 
 // Copyright 2024 Oxide Computer Company
 
-use std::{collections::BTreeMap, marker::PhantomData, net::IpAddr, path::PathBuf};
+use std::{any::TypeId, collections::BTreeMap, marker::PhantomData, net::IpAddr, path::PathBuf};
 
 use anyhow::{bail, Result};
 use async_trait::async_trait;
-use clap::{ArgMatches, Command, CommandFactory, FromArgMatches};
+use clap::{Arg, ArgMatches, Command, CommandFactory, FromArgMatches};
 use log::LevelFilter;
 
 use crate::{
     context::Context,
     generated_cli::{Cli, CliCommand},
     OxideOverride, RunnableCmd,
 };
-use oxide::ClientConfig;
+use oxide::{types::ByteCount, ClientConfig};
 
 /// Control an Oxide environment
 #[derive(clap::Parser, Debug, Clone)]
@@ -614,11 +614,16 @@ impl CommandExt for Command {
                     Command::new(first.to_owned())
                         .subcommand_required(true)
                         .display_order(0)
+                        .mut_args(update_byte_count_help)
                         .add_subcommand(rest, subcmd),
                 )
             }
         } else {
-            let new_subcmd = subcmd.into().name(path.to_owned()).display_order(0);
+            let new_subcmd = subcmd
+                .into()
+                .name(path.to_owned())
+                .display_order(0)
+                .mut_args(update_byte_count_help);
             let has_subcommand = self.find_subcommand(path).is_some();
             if has_subcommand {
                 self.mut_subcommand(path, |cmd| {
@@ -632,6 +637,28 @@ impl CommandExt for Command {
     }
 }
 
+/// For Args that take a `ByteCount`, append a message on the unit formatting accepted and remove
+/// any reference to the field taking bytes as an argument.
+fn update_byte_count_help(arg: Arg) -> Arg {
+    const UNIT_HINT: &str =
+        "unit suffixes are in powers of two (1k = 1024 bytes) for example: 6GiB, 512k, 2048mib";
+
+    let parser = arg.get_value_parser();
+    if parser.type_id() == TypeId::of::<ByteCount>() {
+        let old_help = arg
+            .get_help()
+            .cloned()
+            .map(|h| h.to_string().replace(" (in bytes)", ""));
+
+        arg.help(match old_help {
+            Some(old) => format!("{old}; {UNIT_HINT}"),
+            None => UNIT_HINT.to_string(),
+        })
+    } else {
+        arg
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

cli/src/cmd_instance.rs

@@ -221,27 +221,27 @@ pub struct CmdInstanceFromImage {
     #[clap(long)]
     project: NameOrId,
 
-    /// Description of the instance to create
+    /// Description of the instance
     #[clap(long)]
     description: String,
 
-    /// Hostname of the instance to create
+    /// The hostname to be assigned to the instance
     #[clap(long)]
     hostname: String,
 
-    /// Instance memory e.g 32M. Suffix can be k,m,g,t
+    /// Amount of RAM to be allocated to the instance
     #[clap(long)]
     memory: ByteCount,
 
-    /// Instance CPU count
+    /// Number of vCPUs to be allocated to the instance
     #[clap(long)]
     ncpus: InstanceCpuCount,
 
     /// Source image
     #[clap(long)]
     image: NameOrId,
 
-    /// Boot disk size e.g. 512G. Suffix can be k,m,g,t
+    /// Boot disk size
     #[clap(long)]
     size: ByteCount,