From 27276c10f6125596254904a6cdc11aec86c6c434 Mon Sep 17 00:00:00 2001 From: Abhijit Gadgil Date: Fri, 10 Jan 2025 09:36:28 +0000 Subject: [PATCH] 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 --- components/common/src/cli/clap_validators.rs | 12 ++++++++---- components/core/src/package/ident.rs | 6 ++++++ components/hab/src/cli_v4/pkg/binds.rs | 5 +++-- components/hab/src/cli_v4/pkg/binlink.rs | 5 +++-- components/hab/src/cli_v4/pkg/channels.rs | 5 +++-- components/hab/src/cli_v4/pkg/config.rs | 5 +++-- components/hab/src/cli_v4/pkg/delete.rs | 5 +++-- components/hab/src/cli_v4/pkg/demote.rs | 5 +++-- components/hab/src/cli_v4/pkg/dependencies.rs | 4 +++- components/hab/src/cli_v4/pkg/download.rs | 5 +++-- components/hab/src/cli_v4/pkg/env.rs | 4 +++- components/hab/src/cli_v4/pkg/exec.rs | 4 +++- components/hab/src/cli_v4/pkg/info.rs | 5 +++-- components/hab/src/cli_v4/pkg/path.rs | 4 +++- components/hab/src/cli_v4/pkg/promote.rs | 5 +++-- components/hab/src/cli_v4/pkg/sign.rs | 8 ++++++-- components/hab/src/cli_v4/pkg/uninstall.rs | 5 +++-- components/hab/src/cli_v4/pkg/upload.rs | 5 +++-- components/hab/src/cli_v4/pkg/verify.rs | 5 +++-- 19 files changed, 68 insertions(+), 34 deletions(-) diff --git a/components/common/src/cli/clap_validators.rs b/components/common/src/cli/clap_validators.rs index 88a30decea..fc2c0da008 100644 --- 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 { - 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 index 2af3f1d91b..f05ebf1952 100644 --- a/components/core/src/package/ident.rs +++ b/components/core/src/package/ident.rs @@ -224,6 +224,12 @@ impl FromStr for PackageIdent { } } +impl From 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 index 0493cd83b7..89d22823a8 100644 --- 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 index bb1d611d97..4dc9e768d3 100644 --- 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 index b9c1a68c48..93182a84f0 100644 --- 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 index ba95b81a1a..a90d08d8ce 100644 --- 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 index f9857d06ea..0734f0cef0 100644 --- 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 index 7ff519ac9b..b7f2fece67 100644 --- 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 index 6dc17c0ed7..2a58ab8b53 100644 --- 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 index 1280de3f98..7fc2fc1da2 100644 --- 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, /// 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, /// 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 index c39c719986..32f58ba245 100644 --- 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 index 852ae2c846..d8026d9759 100644 --- 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 index c7963b8740..80467ad8bb 100644 --- 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::::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 index 4422db7772..66b5b17f35 100644 --- 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 index 5d72236c70..e98fe6b24f 100644 --- 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 index 143037ad3d..6365303eed 100644 --- 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, + // 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::((&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::::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 index f480b22c5b..1aef323a22 100644 --- 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 index 98c857661f..f22b134d2a 100644 --- 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, + hart_file: Vec, #[command(flatten)] cache_key_path: CacheKeyPath, @@ -72,7 +73,7 @@ impl PkgUploadOptions { &self.bldr_url.to_string(), &self.channel, &auth_token, - hart_file, + &Into::::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 index 5c66ce7665..9a8d24816e 100644 --- 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::((&self.cache_key_path).into()); - verify::start(ui, &self.source, &key_cache) + verify::start(ui, &Into::::into(self.source.clone()), &key_cache) } }