From a34ab9892c9364acaa4aa26ac2fe7186e1d3345b Mon Sep 17 00:00:00 2001 From: jlanson Date: Thu, 20 Feb 2025 09:09:22 -0500 Subject: [PATCH] feat: improvements between SDK error messages and HC core config error display Signed-off-by: jlanson --- hipcheck/src/main.rs | 2 +- hipcheck/src/plugin/types.rs | 24 +++++++++++++++++++----- plugins/binary/src/main.rs | 15 +++++++++++++-- plugins/github/src/main.rs | 1 - sdk/rust/src/error.rs | 19 ++++++++----------- 5 files changed, 41 insertions(+), 20 deletions(-) diff --git a/hipcheck/src/main.rs b/hipcheck/src/main.rs index ccf77393..614bacce 100644 --- a/hipcheck/src/main.rs +++ b/hipcheck/src/main.rs @@ -626,7 +626,7 @@ fn cmd_ready(config: &CliConfig) { if ready.is_ready() { println!("Hipcheck is ready to run!"); } else { - println!("Hipheck is NOT ready to run"); + println!("Hipcheck is NOT ready to run"); } } diff --git a/hipcheck/src/plugin/types.rs b/hipcheck/src/plugin/types.rs index e712dfe0..159e0eb6 100644 --- a/hipcheck/src/plugin/types.rs +++ b/hipcheck/src/plugin/types.rs @@ -97,18 +97,27 @@ pub enum ConfigErrorType { MissingRequiredConfig = 2, UnrecognizedConfig = 3, InvalidConfigValue = 4, + InternalError = 5, + FileNotFound = 6, + ParseError = 7, + EnvVarNotSet = 8, + MissingProgram = 9, } impl TryFrom for ConfigErrorType { type Error = crate::error::Error; fn try_from(value: ConfigurationStatus) -> Result { - use ConfigErrorType::*; use ConfigurationStatus::*; Ok(match value as i32 { - x if x == Unspecified as i32 => Unknown, - x if x == MissingRequiredConfiguration as i32 => MissingRequiredConfig, - x if x == UnrecognizedConfiguration as i32 => UnrecognizedConfig, - x if x == InvalidConfigurationValue as i32 => InvalidConfigValue, + x if x == Unspecified as i32 => ConfigErrorType::Unknown, + x if x == MissingRequiredConfiguration as i32 => ConfigErrorType::MissingRequiredConfig, + x if x == UnrecognizedConfiguration as i32 => ConfigErrorType::UnrecognizedConfig, + x if x == InvalidConfigurationValue as i32 => ConfigErrorType::InvalidConfigValue, + x if x == InternalError as i32 => ConfigErrorType::InternalError, + x if x == FileNotFound as i32 => ConfigErrorType::FileNotFound, + x if x == ParseError as i32 => ConfigErrorType::ParseError, + x if x == EnvVarNotSet as i32 => ConfigErrorType::EnvVarNotSet, + x if x == MissingProgram as i32 => ConfigErrorType::MissingProgram, x => { return Err(hc_error!("status value '{}' is not an error", x)); } @@ -135,6 +144,11 @@ impl std::fmt::Display for ConfigError { MissingRequiredConfig => "configuration is missing required fields", UnrecognizedConfig => "configuration contains unrecognized fields", InvalidConfigValue => "configuration contains invalid values", + InternalError => "plugin experienced an internal error", + FileNotFound => "a necessary file was not found", + ParseError => "plugin failed to parse data", + EnvVarNotSet => "a necessary environment variable was not set", + MissingProgram => "plugin could not find or run a needed program", }; match &self.message { Some(msg) => write!(f, "{}: {}", err, msg), diff --git a/plugins/binary/src/main.rs b/plugins/binary/src/main.rs index 41043e78..27953442 100644 --- a/plugins/binary/src/main.rs +++ b/plugins/binary/src/main.rs @@ -12,7 +12,7 @@ use hipcheck_sdk::{ }; use pathbuf::pathbuf; use serde::Deserialize; -use std::{path::PathBuf, result::Result as StdResult, sync::OnceLock}; +use std::{ops::Not, path::PathBuf, result::Result as StdResult, sync::OnceLock}; pub static DETECTOR: OnceLock = OnceLock::new(); @@ -92,11 +92,22 @@ impl Plugin for BinaryPlugin { message: "plugin was already configured".to_string(), })?; + if std::fs::exists(&conf.binary_file) + .map_err(|e| ConfigError::InternalError { + message: format!("failed to check file existence: {e}"), + })? + .not() + { + return Err(ConfigError::FileNotFound { + file_path: format!("{}", conf.binary_file.display()), + }); + } + // Use the langs file to create a SourceFileDetector and init the salsa db let bfd = BinaryFileDetector::load(&conf.binary_file).map_err(|e| ConfigError::ParseError { source: format!( - "Binary type definitions file at {}", + "binary file type definitions at {}", conf.binary_file.display() ), message: e.to_string_pretty_multiline(), diff --git a/plugins/github/src/main.rs b/plugins/github/src/main.rs index 0fdbbe3a..adca4e47 100644 --- a/plugins/github/src/main.rs +++ b/plugins/github/src/main.rs @@ -34,7 +34,6 @@ impl TryFrom for Config { if let Some(atv) = value.api_token_var { let api_token = std::env::var(&atv).map_err(|_e| ConfigError::EnvVarNotSet { env_var_name: atv.clone(), - field_name: "api-token-var".to_owned(), purpose: "This environment variable must contain a GitHub API token.".to_owned(), })?; Ok(Config { api_token }) diff --git a/sdk/rust/src/error.rs b/sdk/rust/src/error.rs index 2671b8ae..43dfac3b 100644 --- a/sdk/rust/src/error.rs +++ b/sdk/rust/src/error.rs @@ -156,8 +156,6 @@ pub enum ConfigError { EnvVarNotSet { /// Name of the environment variable env_var_name: String, - /// Config field that set the variable name - field_name: String, /// Message describing what the environment variable should contain purpose: String, }, @@ -175,7 +173,7 @@ impl From for SetConfigurationResponse { reason, } => SetConfigurationResponse { status: ConfigurationStatus::InvalidConfigurationValue as i32, - message: format!("invalid value '{value}' for '{field_name}', reason: '{reason}'"), + message: format!("'{value}' for '{field_name}', reason: '{reason}'"), }, ConfigError::MissingRequiredConfig { field_name, @@ -185,7 +183,7 @@ impl From for SetConfigurationResponse { status: ConfigurationStatus::MissingRequiredConfiguration as i32, message: { let mut message = format!( - "missing required config item '{field_name}' of type '{field_type}'" + "'{field_name}' of type '{field_type}'" ); if possible_values.is_empty().not() { @@ -204,7 +202,7 @@ impl From for SetConfigurationResponse { status: ConfigurationStatus::UnrecognizedConfiguration as i32, message: { let mut message = - format!("unrecognized field '{field_name}' with value '{field_value}'"); + format!("'{field_name}' with value '{field_value}'"); if possible_confusables.is_empty().not() { message.push_str("; possible field names: "); @@ -220,30 +218,29 @@ impl From for SetConfigurationResponse { }, ConfigError::InternalError { message } => SetConfigurationResponse { status: ConfigurationStatus::InternalError as i32, - message: format!("The plugin encountered an error, probably due to incorrect assumptions: {message}"), + message: format!("the plugin encountered an error, probably due to incorrect assumptions: {message}"), }, ConfigError::FileNotFound { file_path } => SetConfigurationResponse { status: ConfigurationStatus::FileNotFound as i32, - message: format!("File not found at path {file_path}"), + message: file_path, }, ConfigError::ParseError { source, message, } => SetConfigurationResponse { status: ConfigurationStatus::ParseError as i32, - message: format!("The plugin's data from \"{source}\" could not be parsed correctly: {message}"), + message: format!("{source} could not be parsed correctly: {message}"), }, ConfigError::EnvVarNotSet { env_var_name, - field_name, purpose, } => SetConfigurationResponse { status: ConfigurationStatus::EnvVarNotSet as i32, - message: format!("Could not find an environment variable with the name \"{env_var_name}\" (set by config field '{field_name}'). Purpose: {purpose}"), + message: format!("\"{env_var_name}\". Purpose: {purpose}"), }, ConfigError::MissingProgram { program_name } => SetConfigurationResponse { status: ConfigurationStatus::MissingProgram as i32, - message: format!("The plugin could not find or run a needed program: {program_name}"), + message: program_name, }, } }