review comments pass-2 : Validators fixes

`HabPkgIdentValueParser` validator was not used for `PackageIdent` which is a struct field in many of the `pkg` commands. Started using it. Fixed String to Pathbuf and String to PackageIdent conversions in the validators Signed-off-by: Abhijit Gadgil <[email protected]>
habitat-sh · Jan 10, 2025 · 27276c1 · 27276c1
1 parent 6804517
commit 27276c1
Show file tree

Hide file tree

Showing 19 changed files with 68 additions and 34 deletions.
diff --git a/components/common/src/cli/clap_validators.rs b/components/common/src/cli/clap_validators.rs
@@ -122,6 +122,8 @@ impl clap_v4::builder::TypedValueParser for FileExistsValueParser {
     }
 }
 
+// TODO: This will be used by `hab config` (this implements the functionality of
+// `file_exists_or_stdin` validator in Clap v2.
 /// Struct implementing validator that validates the valie is a valid 'file' or 'stdin'
 #[derive(Clone)]
 pub struct FileExistsOrStdinValueParser;
@@ -145,14 +147,16 @@ impl clap_v4::builder::TypedValueParser for FileExistsOrStdinValueParser {
 pub struct DirExistsValueParser;
 
 impl clap_v4::builder::TypedValueParser for DirExistsValueParser {
-    type Value = String;
+    type Value = std::path::PathBuf;
 
     fn parse_ref(&self,
                  cmd: &clap_v4::Command,
                  arg: Option<&clap_v4::Arg>,
                  value: &std::ffi::OsStr)
                  -> Result<Self::Value, clap_v4::Error> {
-        parse_ref_internal(cmd, arg, value, true, false, "is not a valid directory")
+        parse_ref_internal(cmd, arg, value, true, false, "is not a valid directory").map(|x| {
+                                                                                        x.into()
+                                                                                    })
     }
 }
 
@@ -277,7 +281,7 @@ use habitat_core::package::ident::{FullyQualifiedPackageIdent,
                                    PackageIdent};
 
 impl clap_v4::builder::TypedValueParser for HabPkgIdentValueParser {
-    type Value = String;
+    type Value = PackageIdent;
 
     fn parse_ref(&self,
                  cmd: &clap_v4::Command,
@@ -305,7 +309,7 @@ impl clap_v4::builder::TypedValueParser for HabPkgIdentValueParser {
                                                                     result.unwrap(),)));
             Err(err)
         } else {
-            Ok(val)
+            Ok(val.into())
         }
     }
 }

diff --git a/components/core/src/package/ident.rs b/components/core/src/package/ident.rs
@@ -224,6 +224,12 @@ impl FromStr for PackageIdent {
     }
 }
 
+impl From<String> for PackageIdent {
+    fn from(ident: String) -> Self {
+        Self::from_str(ident.as_str()).expect("Invalid Package Identifier")
+    }
+}
+
 impl PartialOrd for PackageIdent {
     /// Packages can be compared according to the following:
     ///

diff --git a/components/hab/src/cli_v4/pkg/binds.rs b/components/hab/src/cli_v4/pkg/binds.rs
@@ -7,7 +7,8 @@ use clap::Parser;
 use habitat_core::{fs::FS_ROOT_PATH,
                    package::PackageIdent};
 
-use habitat_common::command::package::binds;
+use habitat_common::{cli::clap_validators::HabPkgIdentValueParser,
+                     command::package::binds};
 
 use crate::error::Result as HabResult;
 
@@ -17,7 +18,7 @@ use crate::error::Result as HabResult;
                            {usage}\n\n{all-args}\n")]
 pub(crate) struct PkgBindsOptions {
     /// A package identifier (ex: core/redis, core/busybox-static/1.42.2)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::simple())]
     pkg_ident: PackageIdent,
 }
 

diff --git a/components/hab/src/cli_v4/pkg/binlink.rs b/components/hab/src/cli_v4/pkg/binlink.rs
@@ -10,7 +10,8 @@ use clap::{ArgAction,
 use habitat_core::{fs::FS_ROOT_PATH,
                    package::PackageIdent};
 
-use habitat_common::{cli::{BINLINK_DIR_ENVVAR,
+use habitat_common::{cli::{clap_validators::HabPkgIdentValueParser,
+                           BINLINK_DIR_ENVVAR,
                            DEFAULT_BINLINK_DIR},
                      ui::UI};
 
@@ -23,7 +24,7 @@ use crate::{command::pkg::binlink,
                            {usage}\n\n{all-args}\n")]
 pub(crate) struct PkgBinlinkOptions {
     /// A package identifier (ex: core/redis, core/busybox-static/1.42.2)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::simple())]
     pkg_ident: PackageIdent,
 
     /// The command to binlink (ex: bash)

diff --git a/components/hab/src/cli_v4/pkg/channels.rs b/components/hab/src/cli_v4/pkg/channels.rs
@@ -4,7 +4,8 @@ use clap_v4 as clap;
 
 use clap::Parser;
 
-use habitat_common::{cli::PACKAGE_TARGET_ENVVAR,
+use habitat_common::{cli::{clap_validators::HabPkgIdentValueParser,
+                           PACKAGE_TARGET_ENVVAR},
                      ui::UI};
 
 use habitat_core::package::{target,
@@ -25,7 +26,7 @@ pub(crate) struct PkgChannelsOptions {
     bldr_url: BldrUrl,
 
     /// A fully qualified package identifier (ex: core/busybox-static/1.42.2/20170513215502)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::full())]
     pkg_ident: PackageIdent,
 
     /// A package target (ex: x86_64-windows) (default: system appropriate target)

diff --git a/components/hab/src/cli_v4/pkg/config.rs b/components/hab/src/cli_v4/pkg/config.rs
@@ -7,7 +7,8 @@ use clap::Parser;
 use habitat_core::{fs::FS_ROOT_PATH,
                    package::PackageIdent};
 
-use habitat_common::command::package::config;
+use habitat_common::{cli::clap_validators::HabPkgIdentValueParser,
+                     command::package::config};
 
 use crate::error::Result as HabResult;
 
@@ -17,7 +18,7 @@ use crate::error::Result as HabResult;
                            {usage}\n\n{all-args}\n")]
 pub(crate) struct PkgConfigOptions {
     /// A package identifier (ex: core/redis, core/busybox-static/1.42.2)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::simple())]
     pkg_ident: PackageIdent,
 }
 

diff --git a/components/hab/src/cli_v4/pkg/delete.rs b/components/hab/src/cli_v4/pkg/delete.rs
@@ -4,7 +4,8 @@ use clap_v4 as clap;
 
 use clap::Parser;
 
-use habitat_common::{cli::PACKAGE_TARGET_ENVVAR,
+use habitat_common::{cli::{clap_validators::HabPkgIdentValueParser,
+                           PACKAGE_TARGET_ENVVAR},
                      ui::UI};
 
 use habitat_core::package::{target,
@@ -25,7 +26,7 @@ pub(crate) struct PkgDeleteOptions {
     bldr_url: BldrUrl,
 
     /// A fully qualified package identifier (ex: core/busybox-static/1.42.2/20170513215502)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::full())]
     pkg_ident: PackageIdent,
 
     /// A package target (ex: x86_64-windows) (default: system appropriate target)

diff --git a/components/hab/src/cli_v4/pkg/demote.rs b/components/hab/src/cli_v4/pkg/demote.rs
@@ -4,7 +4,8 @@ use clap_v4 as clap;
 
 use clap::Parser;
 
-use habitat_common::{cli::PACKAGE_TARGET_ENVVAR,
+use habitat_common::{cli::{clap_validators::HabPkgIdentValueParser,
+                           PACKAGE_TARGET_ENVVAR},
                      ui::UI};
 
 use habitat_core::{package::{target,
@@ -26,7 +27,7 @@ pub(crate) struct PkgDemoteOptions {
     bldr_url: BldrUrl,
 
     /// A fully qualified package identifier (ex: core/busybox-static/1.42.2/20170513215502)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::full())]
     pkg_ident: PackageIdent,
 
     /// Demote from the specified release channel

diff --git a/components/hab/src/cli_v4/pkg/dependencies.rs b/components/hab/src/cli_v4/pkg/dependencies.rs
@@ -5,6 +5,8 @@ use clap_v4 as clap;
 use clap::{ArgAction,
            Parser};
 
+use habitat_common::cli::clap_validators::HabPkgIdentValueParser;
+
 use habitat_core::{fs::FS_ROOT_PATH,
                    package::PackageIdent};
 
@@ -19,7 +21,7 @@ use crate::{command::pkg::{dependencies,
                            {usage}\n\n{all-args}\n")]
 pub(crate) struct PkgDependenciesOptions {
     /// A package identifier (ex: core/redis, core/busybox-static/1.42.2)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::simple())]
     pkg_ident: PackageIdent,
 
     /// Show transitive dependencies

diff --git a/components/hab/src/cli_v4/pkg/download.rs b/components/hab/src/cli_v4/pkg/download.rs
@@ -7,7 +7,8 @@ use std::path::PathBuf;
 use clap::{ArgAction,
            Parser};
 
-use habitat_common::{cli::{clap_validators::TomlOrPkgIdentFileValueParser,
+use habitat_common::{cli::{clap_validators::{HabPkgIdentValueParser,
+                                             TomlOrPkgIdentFileValueParser},
                            file_into_idents,
                            is_toml_file,
                            PACKAGE_TARGET_ENVVAR},
@@ -56,7 +57,7 @@ pub(crate) struct PkgDownloadOptions {
     pkg_ident_file: Vec<String>,
 
     /// One or more Package Identifiers to download (eg. core/redis)
-    #[arg(name = "PKG_IDENT", num_args = 1.., last = true)]
+    #[arg(name = "PKG_IDENT", num_args = 1.., value_parser = HabPkgIdentValueParser::simple())]
     pkg_ident: Vec<PackageIdent>,
 
     /// A package target (ex: x86_64-windows) (default: system appropriate target)

diff --git a/components/hab/src/cli_v4/pkg/env.rs b/components/hab/src/cli_v4/pkg/env.rs
@@ -4,6 +4,8 @@ use clap_v4 as clap;
 
 use clap::Parser;
 
+use habitat_common::cli::clap_validators::HabPkgIdentValueParser;
+
 use habitat_core::{fs::FS_ROOT_PATH,
                    package::PackageIdent};
 
@@ -17,7 +19,7 @@ use crate::error::Result as HabResult;
                            {usage}\n\n{all-args}\n")]
 pub(crate) struct PkgEnvOptions {
     /// A package identifier (ex: core/redis, core/busybox-static/1.42.2)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::simple())]
     pkg_ident: PackageIdent,
 }
 

diff --git a/components/hab/src/cli_v4/pkg/exec.rs b/components/hab/src/cli_v4/pkg/exec.rs
@@ -7,6 +7,8 @@ use std::{ffi::OsString,
 
 use clap::Parser;
 
+use habitat_common::cli::clap_validators::HabPkgIdentValueParser;
+
 use habitat_core::package::PackageIdent;
 
 use crate::{command::pkg::exec,
@@ -18,7 +20,7 @@ use crate::{command::pkg::exec,
                            {usage}\n\n{all-args}\n")]
 pub(crate) struct PkgExecOptions {
     /// A package identifier (ex: core/redis, core/busybox-static/1.42.2)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::simple())]
     pkg_ident: PackageIdent,
 
     /// The command to execute (ex: ls)

diff --git a/components/hab/src/cli_v4/pkg/info.rs b/components/hab/src/cli_v4/pkg/info.rs
@@ -26,15 +26,16 @@ pub(crate) struct PkgInfoOptions {
           action = ArgAction::SetTrue)]
     json: bool,
 
+    // TODO: Move to semantic PathBuf after CLAP-v2 support is removed kept due to Clap V2 quirk
     /// A path to a Habitat Artifact (ex: /home/acme-redis-3.0.7-21120102031201-x86_64-linux.hart)
     #[arg(name = "SOURCE", value_parser = FileExistsValueParser)]
-    source: PathBuf,
+    source: String,
 }
 
 impl PkgInfoOptions {
     pub(super) fn do_info(&self, ui: &mut UI) -> HabResult<()> {
         crypto::init()?;
 
-        info::start(ui, &self.source, self.json)
+        info::start(ui, &Into::<PathBuf>::into(self.source.clone()), self.json)
     }
 }
diff --git a/components/hab/src/cli_v4/pkg/path.rs b/components/hab/src/cli_v4/pkg/path.rs
@@ -4,6 +4,8 @@ use clap_v4 as clap;
 
 use clap::Parser;
 
+use habitat_common::cli::clap_validators::HabPkgIdentValueParser;
+
 use habitat_core::{fs::FS_ROOT_PATH,
                    package::PackageIdent};
 
@@ -16,7 +18,7 @@ use crate::{command::pkg::path,
                            {usage}\n\n{all-args}\n")]
 pub(crate) struct PkgPathOptions {
     /// A package identifier (ex: core/redis, core/busybox-static/1.42.2)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::simple())]
     pkg_ident: PackageIdent,
 }
 

diff --git a/components/hab/src/cli_v4/pkg/promote.rs b/components/hab/src/cli_v4/pkg/promote.rs
@@ -4,7 +4,8 @@ use clap_v4 as clap;
 
 use clap::Parser;
 
-use habitat_common::{cli::PACKAGE_TARGET_ENVVAR,
+use habitat_common::{cli::{clap_validators::HabPkgIdentValueParser,
+                           PACKAGE_TARGET_ENVVAR},
                      ui::UI};
 
 use habitat_core::{package::{target,
@@ -26,7 +27,7 @@ pub(crate) struct PkgPromoteOptions {
     bldr_url: BldrUrl,
 
     /// A fully qualified package identifier (ex: core/busybox-static/1.42.2/20170513215502)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::full())]
     pkg_ident: PackageIdent,
 
     /// Promote from the specified release channel

diff --git a/components/hab/src/cli_v4/pkg/sign.rs b/components/hab/src/cli_v4/pkg/sign.rs
@@ -29,9 +29,10 @@ pub(crate) struct PkgSignOptions {
     #[arg(name = "ORIGIN", long = "origin", env=crate::ORIGIN_ENVVAR, value_parser = HabOriginValueParser)]
     origin: Option<Origin>,
 
+    // TODO: Move to semantic PathBuf after CLAP-v2 support is removed kept due to Clap V2 quirk
     /// A path to a source archive file (ex: /home/acme-redis-3.0.7-21120102031201.tar.xz)
     #[arg(name = "SOURCE", value_parser = FileExistsValueParser)]
-    source: PathBuf,
+    source: String,
 
     /// The destination path to the signed Habitat Artifact (ex:
     /// /home/acme-redis-3.0.7-21120102031201-x86_64-linux.hart)
@@ -56,6 +57,9 @@ impl PkgSignOptions {
         crypto::init()?;
         let key_cache = KeyCache::new::<PathBuf>((&self.cache_key_path).into());
         let key = key_cache.latest_secret_origin_signing_key(&origin)?;
-        sign::start(ui, &key, &self.source, &self.dest)
+        sign::start(ui,
+                    &key,
+                    &Into::<PathBuf>::into(self.source.clone()),
+                    &self.dest)
     }
 }
diff --git a/components/hab/src/cli_v4/pkg/uninstall.rs b/components/hab/src/cli_v4/pkg/uninstall.rs
@@ -7,7 +7,8 @@ use clap::{ArgAction,
 use habitat_core::{fs::FS_ROOT_PATH,
                    package::PackageIdent};
 
-use habitat_common::ui::UI;
+use habitat_common::{cli::clap_validators::HabPkgIdentValueParser,
+                     ui::UI};
 
 use crate::{command::pkg::{uninstall,
                            uninstall::UninstallHookMode,
@@ -21,7 +22,7 @@ use crate::{command::pkg::{uninstall,
                            {usage}\n\n{all-args}\n")]
 pub(crate) struct PkgUninstallOptions {
     /// A package identifier (ex: core/redis, core/busybox-static/1.42.2)
-    #[arg(name = "PKG_IDENT")]
+    #[arg(name = "PKG_IDENT", value_parser = HabPkgIdentValueParser::simple())]
     pkg_ident: PackageIdent,
 
     /// Just show what would be uninstalled, don't actually do it

diff --git a/components/hab/src/cli_v4/pkg/upload.rs b/components/hab/src/cli_v4/pkg/upload.rs
@@ -46,10 +46,11 @@ pub(crate) struct PkgUploadOptions {
     #[arg(name = "NO_BUILD", long = "no-build", action = ArgAction::SetTrue)]
     no_build: bool,
 
+    // TODO: Move to semantic PathBuf after CLAP-v2 support is removed kept due to Clap V2 quirk
     /// One or more filepaths to a Habitat Artifact (ex:
     /// /home/acme-redis-3.0.7-21120102031201-x86_64-linux.hart)
     #[arg(name = "HART_FILE", required = true, value_parser = FileExistsValueParser)]
-    hart_file: Vec<PathBuf>,
+    hart_file: Vec<String>,
 
     #[command(flatten)]
     cache_key_path: CacheKeyPath,
@@ -72,7 +73,7 @@ impl PkgUploadOptions {
                           &self.bldr_url.to_string(),
                           &self.channel,
                           &auth_token,
-                          hart_file,
+                          &Into::<PathBuf>::into(hart_file.clone()),
                           self.force,
                           auto_build,
                           &key_cache).await?;

diff --git a/components/hab/src/cli_v4/pkg/verify.rs b/components/hab/src/cli_v4/pkg/verify.rs
@@ -21,9 +21,10 @@ use crate::{cli_v4::utils::CacheKeyPath,
           help_template = "{name} {version} {author-section} {about-section} \n{usage-heading} \
                            {usage}\n\n{all-args}\n")]
 pub(crate) struct PkgVerifyOptions {
+    // TODO: Move to semantic PathBuf once Clap-v2 is removed
     /// A path to a Habitat Artifact (ex: /home/acme-redis-3.0.7-21120102031201-x86_64-linux.hart)
     #[arg(name = "SOURCE", value_parser = FileExistsValueParser)]
-    source: PathBuf,
+    source: String,
 
     #[command(flatten)]
     cache_key_path: CacheKeyPath,
@@ -34,6 +35,6 @@ impl PkgVerifyOptions {
         crypto::init()?;
         let key_cache = KeyCache::new::<PathBuf>((&self.cache_key_path).into());
 
-        verify::start(ui, &self.source, &key_cache)
+        verify::start(ui, &Into::<PathBuf>::into(self.source.clone()), &key_cache)
     }
 }