Skip to content

Commit

Permalink
feat: improvements between SDK error messages and HC core config erro…
Browse files Browse the repository at this point in the history
…r display

Signed-off-by: jlanson <[email protected]>
  • Loading branch information
j-lanson committed Feb 20, 2025
1 parent 69efa8c commit a34ab98
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 20 deletions.
2 changes: 1 addition & 1 deletion hipcheck/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}

Expand Down
24 changes: 19 additions & 5 deletions hipcheck/src/plugin/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<ConfigurationStatus> for ConfigErrorType {
type Error = crate::error::Error;
fn try_from(value: ConfigurationStatus) -> Result<Self> {
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));
}
Expand All @@ -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),
Expand Down
15 changes: 13 additions & 2 deletions plugins/binary/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<BinaryFileDetector> = OnceLock::new();

Expand Down Expand Up @@ -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(),
Expand Down
1 change: 0 additions & 1 deletion plugins/github/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ impl TryFrom<RawConfig> 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 })
Expand Down
19 changes: 8 additions & 11 deletions sdk/rust/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand All @@ -175,7 +173,7 @@ impl From<ConfigError> 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,
Expand All @@ -185,7 +183,7 @@ impl From<ConfigError> 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() {
Expand All @@ -204,7 +202,7 @@ impl From<ConfigError> 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: ");
Expand All @@ -220,30 +218,29 @@ impl From<ConfigError> 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,
},
}
}
Expand Down

0 comments on commit a34ab98

Please sign in to comment.