[2/n] [omicron-package] better error messages around target

package/src/bin/omicron-package.rs

@@ -12,7 +12,7 @@ use illumos_utils::{zfs, zone};
 use indicatif::{MultiProgress, ProgressBar, ProgressStyle};
 use omicron_package::cargo_plan::build_cargo_plan;
 use omicron_package::config::{Config, ConfigArgs};
-use omicron_package::target::KnownTarget;
+use omicron_package::target::{target_command_help, KnownTarget};
 use omicron_package::{parse, BuildCommand, DeployCommand, TargetCommand};
 use omicron_zone_package::config::Config as PackageConfig;
 use omicron_zone_package::package::{Package, PackageOutput, PackageSource};
@@ -157,7 +157,7 @@ async fn do_list_outputs(
 
 async fn do_target(
     artifact_dir: &Utf8Path,
-    name: &str,
+    name: Option<&str>,
     subcommand: &TargetCommand,
 ) -> Result<()> {
     let target_dir = artifact_dir.join("target");
@@ -180,7 +180,7 @@ async fn do_target(
                 clickhouse_topology.clone(),
             )?;
 
-            let path = get_single_target(&target_dir, name).await?;
+            let (name, path) = get_required_named_target(&target_dir, name)?;
             tokio::fs::write(&path, Target::from(target).to_string())
                 .await
                 .with_context(|| {
@@ -215,31 +215,46 @@ async fn do_target(
             }
         }
         TargetCommand::Set => {
-            let _ = get_single_target(&target_dir, name).await?;
+            let (name, _) = get_required_named_target(&target_dir, name)?;
             replace_active_link(&name, &target_dir).await?;
             println!("Set build target '{name}' as active");
         }
         TargetCommand::Delete => {
-            let path = get_single_target(&target_dir, name).await?;
-            tokio::fs::remove_file(&path).await?;
+            let (name, path) = get_required_named_target(&target_dir, name)?;
+            tokio::fs::remove_file(&path).await.with_context(|| {
+                format!("failed to remove target file {}", path)
+            })?;
             println!("Removed build target '{name}'");
         }
     };
     Ok(())
 }
 
-async fn get_single_target(
+/// Get the path to a named target as required by the `target` subcommand.
+///
+/// This function bans `active` as a target name, as it is reserved for the
+/// active target.
+fn get_required_named_target(
     target_dir: impl AsRef<Utf8Path>,
-    name: &str,
-) -> Result<Utf8PathBuf> {
-    if name == Config::ACTIVE {
-        bail!(
-            "The name '{name}' is reserved, please try another (e.g. 'default')\n\
-            Usage: '{} -t <TARGET> target ...'",
-            env::current_exe().unwrap().display(),
-        );
+    name: Option<&str>,
+) -> Result<(&str, Utf8PathBuf)> {
+    match name {
+        Some(name) if name == Config::ACTIVE => {
+            bail!(
+                "the name '{name}' is reserved, please try another (e.g. 'default')\n\
+                 Usage: {} ...",
+                target_command_help("<TARGET>"),
+            );
+        }
+        Some(name) => Ok((name, target_dir.as_ref().join(name))),
+        None => {
+            bail!(
+                "a target name is required for this operation (e.g. 'default')\n\
+                 Usage: {} ...",
+                target_command_help("<TARGET>"),
+            );
+        }
     }
-    Ok(target_dir.as_ref().join(name))
 }
 
 async fn replace_active_link(
@@ -887,7 +902,7 @@ async fn main() -> Result<()> {
         SubCommand::Build(BuildCommand::Target { subcommand }) => {
             do_target(
                 &args.artifact_dir,
-                &args.config_args.target,
+                args.config_args.target.as_deref(),
                 &subcommand,
             )
             .await?;

package/src/config.rs

@@ -11,21 +11,15 @@ use omicron_zone_package::{
     target::Target,
 };
 use slog::{debug, Logger};
-use std::{
-    collections::BTreeMap, env, io::Write, str::FromStr, time::Duration,
-};
+use std::{collections::BTreeMap, io::Write, str::FromStr, time::Duration};
 
-use crate::target::KnownTarget;
+use crate::target::{target_command_help, KnownTarget};
 
 #[derive(Debug, Args)]
 pub struct ConfigArgs {
     /// The name of the build target to use for this command
-    #[clap(
-        short,
-        long,
-        default_value_t = Config::ACTIVE.to_string(),
-    )]
-    pub target: String,
+    #[clap(short, long)]
+    pub target: Option<String>,
 
     /// Skip confirmation prompt for destructive operations
     #[clap(short, long, action, default_value_t = false)]
@@ -78,14 +72,17 @@ impl Config {
         args: &ConfigArgs,
         artifact_dir: &Utf8Path,
     ) -> Result<Self> {
+        // Within this path, the target is expected to be set.
+        let target = args.target.as_deref().unwrap_or(Self::ACTIVE);
+
         let target_help_str = || -> String {
             format!(
-                "Try calling: '{} -t default target create' to create a new build target",
-                env::current_exe().unwrap().display()
+                "Try calling: '{} target create' to create a new build target",
+                target_command_help("default"),
             )
         };
 
-        let target_path = artifact_dir.join("target").join(&args.target);
+        let target_path = artifact_dir.join("target").join(target);
         let raw_target =
             std::fs::read_to_string(&target_path).inspect_err(|_| {
                 eprintln!(
@@ -103,7 +100,7 @@ impl Config {
                 );
             })?
             .into();
-        debug!(log, "target[{}]: {:?}", args.target, target);
+        debug!(log, "target[{}]: {:?}", target, target);
 
         Ok(Config {
             log: log.clone(),

package/src/target.rs

@@ -200,3 +200,11 @@ impl std::str::FromStr for KnownTarget {
         )
     }
 }
+
+/// Generate a command to build a target, for use in usage strings.
+pub fn target_command_help(target_name: &str) -> String {
+    format!(
+        "{} -t {target_name} target",
+        std::env::current_exe().unwrap().display(),
+    )
+}